[jira] [Comment Edited] (OFBIZ-5761) Allow to edit ship groups contents after and order has been created
[ https://issues.apache.org/jira/browse/OFBIZ-5761?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanelfocusedCommentId=14226104#comment-14226104 ] Jacques Le Roux edited comment on OFBIZ-5761 at 11/26/14 12:08 PM: --- Hi Nicolas, I reviewed (by patches comparison, boring but sure) and tested your last patch. It's quite good. I like how you cleaned the code and I like the UI also. I found only one issue where deleteOrderItemShipGroupAssoc needs a userLogin. {code} -+MapString, String localCtx = UtilMisc.toMap(orderId, orderItem.getString(orderId), ++MapString, Object localCtx = UtilMisc.toMap(userLogin, userLogin, ++orderId, orderItem.getString(orderId), I found some other trivial points, you can see them in the attached OFBIZ-5761 - OISG Management.patch.diff file. I notably removed the duplicate create new ship group buttons. I kept the original one, but it now uses the new AddOrderItemShipGroup service. {code} I attach also the last OFBIZ-5761 - OISG Management.patch. Before I commit, I will add a Junit test and will change the delegator.* calls to EntityQuery.* calls. That should be it :) was (Author: jacques.le.roux): Hi Nicolas, I reviewed (by patches comparison, boring but sure) and tested your last patch. It's quite good. I like how you cleaned the code and I like the UI also. I found only one issue where deleteOrderItemShipGroupAssoc needs a userLogin. {code} -+MapString, String localCtx = UtilMisc.toMap(orderId, orderItem.getString(orderId), ++MapString, Object localCtx = UtilMisc.toMap(userLogin, userLogin, ++orderId, orderItem.getString(orderId), I found some other trivial points, you can see them in the attached OFBIZ-5761 - OISG Management.patch.diff file. I notably removed the duplicate create new ship group buttons. I kept the original one, but it now uses the new AddOrderItemShipGroup service. I attach also the last OFBIZ-5761 - OISG Management.patch. Before I commit, I will add a Junit test and will change the delegator.* calls to EntityQuery.* calls. That should be it :) 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, OFBIZ-5761 - OISG Management.patch, OFBIZ-5761 - OISG Management.patch, OFBIZ-5761 - OISG Management.patch, OFBIZ-5761 - OISG Management.patch, OFBIZ-5761 - OISG Management.patch, OFBIZ-5761 - OISG Management.patch.change, OFBIZ-5761 - OISG Management.patch.diff, OFBIZ-5761.patch.diff 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 thats 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)
[jira] [Comment Edited] (OFBIZ-5761) Allow to edit ship groups contents after and order has been created
[ https://issues.apache.org/jira/browse/OFBIZ-5761?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanelfocusedCommentId=14226104#comment-14226104 ] Jacques Le Roux edited comment on OFBIZ-5761 at 11/26/14 12:09 PM: --- Hi Nicolas, I reviewed (by patches comparison, boring but sure) and tested your last patch. It's quite good. I like how you cleaned the code and I like the UI also. I found only one issue where deleteOrderItemShipGroupAssoc needs a userLogin. {code} -+MapString, String localCtx = UtilMisc.toMap(orderId, orderItem.getString(orderId), ++MapString, Object localCtx = UtilMisc.toMap(userLogin, userLogin, ++orderId, orderItem.getString(orderId), {code} I found some other trivial points, you can see them in the attached OFBIZ-5761 - OISG Management.patch.diff file. I notably removed the duplicate create new ship group button. I kept the original one, but it now uses the new AddOrderItemShipGroup service. I attach also the last OFBIZ-5761 - OISG Management.patch. Before I commit, I will add a Junit test and will change the delegator.* calls to EntityQuery.* calls. That should be it :) was (Author: jacques.le.roux): Hi Nicolas, I reviewed (by patches comparison, boring but sure) and tested your last patch. It's quite good. I like how you cleaned the code and I like the UI also. I found only one issue where deleteOrderItemShipGroupAssoc needs a userLogin. {code} -+MapString, String localCtx = UtilMisc.toMap(orderId, orderItem.getString(orderId), ++MapString, Object localCtx = UtilMisc.toMap(userLogin, userLogin, ++orderId, orderItem.getString(orderId), {code} I found some other trivial points, you can see them in the attached OFBIZ-5761 - OISG Management.patch.diff file. I notably removed the duplicate create new ship group buttons. I kept the original one, but it now uses the new AddOrderItemShipGroup service. I attach also the last OFBIZ-5761 - OISG Management.patch. Before I commit, I will add a Junit test and will change the delegator.* calls to EntityQuery.* calls. That should be it :) 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, OFBIZ-5761 - OISG Management.patch, OFBIZ-5761 - OISG Management.patch, OFBIZ-5761 - OISG Management.patch, OFBIZ-5761 - OISG Management.patch, OFBIZ-5761 - OISG Management.patch, OFBIZ-5761 - OISG Management.patch.change, OFBIZ-5761 - OISG Management.patch.diff, OFBIZ-5761.patch.diff 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 thats 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)
[jira] [Comment Edited] (OFBIZ-5761) Allow to edit ship groups contents after and order has been created
[ https://issues.apache.org/jira/browse/OFBIZ-5761?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanelfocusedCommentId=14226104#comment-14226104 ] Jacques Le Roux edited comment on OFBIZ-5761 at 11/26/14 12:09 PM: --- Hi Nicolas, I reviewed (by patches comparison, boring but sure) and tested your last patch. It's quite good. I like how you cleaned the code and I like the UI also. I found only one issue where deleteOrderItemShipGroupAssoc needs a userLogin. {code} -+MapString, String localCtx = UtilMisc.toMap(orderId, orderItem.getString(orderId), ++MapString, Object localCtx = UtilMisc.toMap(userLogin, userLogin, ++orderId, orderItem.getString(orderId), {code} I found some other trivial points, you can see them in the attached OFBIZ-5761 - OISG Management.patch.diff file. I notably removed the duplicate create new ship group buttons. I kept the original one, but it now uses the new AddOrderItemShipGroup service. I attach also the last OFBIZ-5761 - OISG Management.patch. Before I commit, I will add a Junit test and will change the delegator.* calls to EntityQuery.* calls. That should be it :) was (Author: jacques.le.roux): Hi Nicolas, I reviewed (by patches comparison, boring but sure) and tested your last patch. It's quite good. I like how you cleaned the code and I like the UI also. I found only one issue where deleteOrderItemShipGroupAssoc needs a userLogin. {code} -+MapString, String localCtx = UtilMisc.toMap(orderId, orderItem.getString(orderId), ++MapString, Object localCtx = UtilMisc.toMap(userLogin, userLogin, ++orderId, orderItem.getString(orderId), I found some other trivial points, you can see them in the attached OFBIZ-5761 - OISG Management.patch.diff file. I notably removed the duplicate create new ship group buttons. I kept the original one, but it now uses the new AddOrderItemShipGroup service. {code} I attach also the last OFBIZ-5761 - OISG Management.patch. Before I commit, I will add a Junit test and will change the delegator.* calls to EntityQuery.* calls. That should be it :) 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, OFBIZ-5761 - OISG Management.patch, OFBIZ-5761 - OISG Management.patch, OFBIZ-5761 - OISG Management.patch, OFBIZ-5761 - OISG Management.patch, OFBIZ-5761 - OISG Management.patch, OFBIZ-5761 - OISG Management.patch.change, OFBIZ-5761 - OISG Management.patch.diff, OFBIZ-5761.patch.diff 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 thats 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)
[jira] [Comment Edited] (OFBIZ-5761) Allow to edit ship groups contents after and order has been created
[ https://issues.apache.org/jira/browse/OFBIZ-5761?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanelfocusedCommentId=14172487#comment-14172487 ] Jacques Le Roux edited comment on OFBIZ-5761 at 10/23/14 8:13 AM: -- Just as a note here for now. In my custom code I today replaced several Debug.log() introduced with Neogia addon with Debug.logInfo() The interesting aspect is I found them because I created an error.log with log4j2 (for OFBIZ-5771) and was surprises to see a bunch of FATAL lines, they came from Debug.log() was (Author: jacques.le.roux): Just as a note here for now. In my custom code I today replaced several Debug.log() introduced with Neogia addon with Debug.logInfo() The interesting aspect is I found them because I created an error.log with log4j2 (for OFBIZ-4468) and was surprises to see a bunch of FATAL lines, they came from Debug.log() 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, OFBIZ-5761 - OISG Management.patch, OFBIZ-5761 - OISG Management.patch, OFBIZ-5761 - OISG Management.patch, OFBIZ-5761 - OISG Management.patch, OFBIZ-5761 - OISG Management.patch.change, OFBIZ-5761.patch.diff 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 thats 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)
[jira] [Comment Edited] (OFBIZ-5761) Allow to edit ship groups contents after and order has been created
[ https://issues.apache.org/jira/browse/OFBIZ-5761?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanelfocusedCommentId=14168772#comment-14168772 ] Nicolas Malin edited comment on OFBIZ-5761 at 10/12/14 8:05 PM: Jacques, I finished the review. * remove estimatedShipDate that use only by OFBiz, by shipByDate * review the IHM to use OFBiz class was (Author: soledad): Jacques, I finish the review. * remove estimatedShipDate that use only by OFBiz by shipByDate * review the IHM to use OFBiz class 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, OFBIZ-5761 - OISG Management.patch, OFBIZ-5761 - OISG Management.patch, OFBIZ-5761 - OISG Management.patch, OFBIZ-5761 - OISG Management.patch, OFBIZ-5761 - OISG Management.patch.change, OFBIZ-5761.patch.diff 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 thats 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)
[jira] [Comment Edited] (OFBIZ-5761) Allow to edit ship groups contents after and order has been created
[ https://issues.apache.org/jira/browse/OFBIZ-5761?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanelfocusedCommentId=14143889#comment-14143889 ] Nicolas Malin edited comment on OFBIZ-5761 at 9/22/14 9:52 PM: --- First step, I review (recode) the service java. Remove unused code, oldest code and rewrite some part to slim that. I will continue to the ftl and pass a second time on service to be sure :) was (Author: soledad): First step, I review (recode) the service java. I will continue to the ftl. 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, OFBIZ-5761 - OISG Management.patch, OFBIZ-5761 - OISG Management.patch, OFBIZ-5761 - OISG Management.patch.change, OFBIZ-5761.patch.diff 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 thats 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)
[jira] [Comment Edited] (OFBIZ-5761) Allow to edit ship groups contents after and order has been created
[ https://issues.apache.org/jira/browse/OFBIZ-5761?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanelfocusedCommentId=14130655#comment-14130655 ] Jacques Le Roux edited comment on OFBIZ-5761 at 9/11/14 8:32 PM: - Mmm, some time ago I noticed something I since forgot to tackle in addOrderItemShipGroup service. I guess because of setEstimatedDeliveryDates service (a ProductionRunService) and initMrpEvents, the addon had this block {code} //shipByDate is mandatory if (context.get(estimatedShipDate) == null) { context.put(shipByDate, UtilDateTime.nowTimestamp()); } else { context.put(shipByDate, context.get(estimatedShipDate)); } // set estimatedDeliveryDate with estimatedShipDate context.put(estimatedDeliveryDate, context.get(estimatedShipDate)); {code} The shipByDate field is *NOT mandatory* and why would we want to ship now? This does not make any sense to me. I guess it was needed in older OFBiz versions of OFBiz manufacturing, but is no longer (I checked). We could set now to shipAfterDate, but I see no reasons to force one or the other. I thought about adapting that block in a new patch with this block: {code} // set estimatedDeliveryDate and shipByDate with estimatedShipDate if it exists Timestamp estimatedShipDate = (Timestamp) context.get(estimatedShipDate); if (estimatedShipDate != null) { context.put(estimatedDeliveryDate, estimatedShipDate); context.put(shipByDate, estimatedShipDate); } {code} But when you look at it there are no existing ways to pass an estimatedShipDate to the addOrderItemShipGroup service. So I believe we should simply remove the block, opinions? was (Author: jacques.le.roux): Mmm, some time ago I noticed something I since forgot to tackle in addOrderItemShipGroup service. I guess because of setEstimatedDeliveryDates service (a ProductionRunService) and initMrpEvents, the addon had this block {code} //shipByDate is mandatory if (context.get(estimatedShipDate) == null) { context.put(shipByDate, UtilDateTime.nowTimestamp()); } else { context.put(shipByDate, context.get(estimatedShipDate)); } // set estimatedDeliveryDate with estimatedShipDate context.put(estimatedDeliveryDate, context.get(estimatedShipDate)); {code} The shipByDate field is *NOT mandatory* and why would we want to ship now? This does not make any sense to me. I guess it was needed in older OFBiz versions of OFBiz manufacturing, but is no longer (I checked). We could set now to shipAfterDate, but I see no reasons to force one or the other. I thought about adapting that block in a new patch with this block: {code} // set estimatedDeliveryDate and shipByDate with estimatedShipDate if it exists if (context.get(estimatedShipDate) == null) { context.put(estimatedDeliveryDate, context.get(cv)); context.put(shipByDate, context.get(estimatedShipDate)); } {code} But when you look at it there are no existing ways to pass an estimatedShipDate to the addOrderItemShipGroup service. So I believe we should simply remove the block, opinions? 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, OFBIZ-5761 - OISG Management.patch, OFBIZ-5761 - OISG Management.patch.change 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
[jira] [Comment Edited] (OFBIZ-5761) Allow to edit ship groups contents after and order has been created
[ https://issues.apache.org/jira/browse/OFBIZ-5761?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanelfocusedCommentId=14126966#comment-14126966 ] Jacques Le Roux edited comment on OFBIZ-5761 at 9/9/14 1: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-, 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. 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 be 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 thats 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
[jira] [Comment Edited] (OFBIZ-5761) Allow to edit ship groups contents after and order has been created
[ https://issues.apache.org/jira/browse/OFBIZ-5761?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanelfocusedCommentId=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 thats 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