Re: svn commit: r1838320 - in /ofbiz/ofbiz-framework/trunk/applications: datamodel/data/seed/OrderSeedData.xml datamodel/data/seed/ProductSeedData.xml order/src/main/java/org/apache/ofbiz/order/shoppingcart/ShoppingCartItem.java

Previous Topic Next Topic
 
classic Classic list List threaded Threaded
10 messages Options
Reply | Threaded
Open this post in threaded view
|

Re: svn commit: r1838320 - in /ofbiz/ofbiz-framework/trunk/applications: datamodel/data/seed/OrderSeedData.xml datamodel/data/seed/ProductSeedData.xml order/src/main/java/org/apache/ofbiz/order/shoppingcart/ShoppingCartItem.java

Pierre Smits-3
Hi Suraj,

Please revert! Within 10 minutes you posted a new patch and committed it to
trunk and closed the issue. It is customary to follow the 72 hr delay rule
to allow the community to review the changes and assess the impact.


Best regards,

Pierre Smits

Apache Trafodion <https://trafodion.apache.org>, Vice President
Apache Directory <https://directory.apache.org>, PMC Member
Apache Incubator <https://incubator.apache.org>, committer
Apache OFBiz <https://ofbiz.apache.org>, contributor since 2008
Apache Steve <https://steve.apache.org>, committer

On Sat, Aug 18, 2018 at 10:34 AM, <[hidden email]> wrote:

> Author: surajk
> Date: Sat Aug 18 08:34:18 2018
> New Revision: 1838320
>
> URL: http://svn.apache.org/viewvc?rev=1838320&view=rev
> Log:
> Improved: Added support to calculate deposit price as well while creating
> shopping cart item.
> (OFBIZ-7482)
>
> Modified:
>     ofbiz/ofbiz-framework/trunk/applications/datamodel/data/
> seed/OrderSeedData.xml
>     ofbiz/ofbiz-framework/trunk/applications/datamodel/data/
> seed/ProductSeedData.xml
>     ofbiz/ofbiz-framework/trunk/applications/order/src/main/
> java/org/apache/ofbiz/order/shoppingcart/ShoppingCartItem.java
>
> Modified: ofbiz/ofbiz-framework/trunk/applications/datamodel/data/
> seed/OrderSeedData.xml
> URL: http://svn.apache.org/viewvc/ofbiz/ofbiz-framework/trunk/
> applications/datamodel/data/seed/OrderSeedData.xml?rev=
> 1838320&r1=1838319&r2=1838320&view=diff
> ============================================================
> ==================
> --- ofbiz/ofbiz-framework/trunk/applications/datamodel/data/seed/OrderSeedData.xml
> (original)
> +++ ofbiz/ofbiz-framework/trunk/applications/datamodel/data/seed/OrderSeedData.xml
> Sat Aug 18 08:34:18 2018
> @@ -49,6 +49,7 @@ under the License.
>      <OrderAdjustmentType description="Additional Feature" hasTable="N"
> orderAdjustmentTypeId="ADDITIONAL_FEATURE"/>
>      <OrderAdjustmentType description="Warranty" hasTable="N"
> orderAdjustmentTypeId="WARRANTY_ADJUSTMENT"/>
>      <OrderAdjustmentType description="Marketing Package Adjustment"
> hasTable="N" orderAdjustmentTypeId="MKTG_PKG_AUTO_ADJUST"/>
> +    <OrderAdjustmentType description="Deposit" hasTable="N"
> orderAdjustmentTypeId="DEPOSIT_ADJUSTMENT"/>
>
>      <OrderBlacklistType orderBlacklistTypeId="BLACKLIST_ADDRESS"
> description="Addresss"/>
>      <OrderBlacklistType orderBlacklistTypeId="BLACKLIST_CREDITCARD"
> description="Credit Card"/>
>
> Modified: ofbiz/ofbiz-framework/trunk/applications/datamodel/data/
> seed/ProductSeedData.xml
> URL: http://svn.apache.org/viewvc/ofbiz/ofbiz-framework/trunk/
> applications/datamodel/data/seed/ProductSeedData.xml?rev=
> 1838320&r1=1838319&r2=1838320&view=diff
> ============================================================
> ==================
> --- ofbiz/ofbiz-framework/trunk/applications/datamodel/data/seed/ProductSeedData.xml
> (original)
> +++ ofbiz/ofbiz-framework/trunk/applications/datamodel/data/seed/ProductSeedData.xml
> Sat Aug 18 08:34:18 2018
> @@ -281,6 +281,7 @@ under the License.
>      <ProductPriceType description="Minimum Order Price"
> productPriceTypeId="MINIMUM_ORDER_PRICE"/>
>      <ProductPriceType description="Shipping Allowance Price"
> productPriceTypeId="SHIPPING_ALLOWANCE"/>
>
> +    <ProductPricePurpose description="Deposit price"
> productPricePurposeId="DEPOSIT"/>
>      <ProductPricePurpose description="Purchase/Initial"
> productPricePurposeId="PURCHASE"/>
>      <ProductPricePurpose description="Recurring Charge"
> productPricePurposeId="RECURRING_CHARGE"/>
>      <ProductPricePurpose description="Usage Charge"
> productPricePurposeId="USAGE_CHARGE"/>
>
> Modified: ofbiz/ofbiz-framework/trunk/applications/order/src/main/
> java/org/apache/ofbiz/order/shoppingcart/ShoppingCartItem.java
> URL: http://svn.apache.org/viewvc/ofbiz/ofbiz-framework/trunk/
> applications/order/src/main/java/org/apache/ofbiz/order/
> shoppingcart/ShoppingCartItem.java?rev=1838320&r1=1838319&
> r2=1838320&view=diff
> ============================================================
> ==================
> --- ofbiz/ofbiz-framework/trunk/applications/order/src/main/
> java/org/apache/ofbiz/order/shoppingcart/ShoppingCartItem.java (original)
> +++ ofbiz/ofbiz-framework/trunk/applications/order/src/main/
> java/org/apache/ofbiz/order/shoppingcart/ShoppingCartItem.java Sat Aug 18
> 08:34:18 2018
> @@ -1038,6 +1038,7 @@ public class ShoppingCartItem implements
>              ProductPromoWorker.doPromotions(cart, dispatcher);
>          }
>
> +        calcDepositAdjustments();
>          if (!"PURCHASE_ORDER".equals(cart.getOrderType())) {
>              // store the auto-save cart
>              if (triggerExternalOps && ProductStoreWorker.autoSaveCart(delegator,
> productStoreId)) {
> @@ -1061,6 +1062,33 @@ public class ShoppingCartItem implements
>          }
>      }
>
> +    public void calcDepositAdjustments() {
> +        List<GenericValue>itemAdjustments = this.getAdjustments();
> +        try {
> +            GenericValue depositAmount = EntityQuery.use(delegator).
> from("ProductPrice").where("productId", this.getProductId(),
> "productPricePurposeId", "DEPOSIT", "productPriceTypeId",
> "DEFAULT_PRICE").filterByDate().queryFirst();
> +            if (UtilValidate.isNotEmpty(depositAmount)) {
> +                Boolean updatedDepositAmount = false;
> +                BigDecimal adjustmentAmount =
> depositAmount.getBigDecimal("price").multiply(this.getQuantity(),
> generalRounding);
> +                // itemAdjustments is a reference so directly setting
> updated amount to the same.
> +                    for(GenericValue itemAdjustment : itemAdjustments) {
> +                    if("DEPOSIT_ADJUSTMENT".equals(itemAdjustment.
> getString("orderAdjustmentTypeId"))) {
> +                            itemAdjustment.set("amount",
> adjustmentAmount);
> +                            updatedDepositAmount = true;
> +                        }
> +                }
> +                if (!updatedDepositAmount) {
> +                    GenericValue orderAdjustment = delegator.makeValue("
> OrderAdjustment");
> +                    orderAdjustment.set("orderAdjustmentTypeId",
> "DEPOSIT_ADJUSTMENT");
> +                    orderAdjustment.set("description", "Surcharge
> Adjustment");
> +                    orderAdjustment.set("amount", adjustmentAmount);
> +                    this.addAdjustment(orderAdjustment);
> +                }
> +            }
> +        } catch (GenericEntityException e){
> +            Debug.logError("Error in fetching deposite price details!!",
> module);
> +        }
> +    }
> +
>      public void updatePrice(LocalDispatcher dispatcher, ShoppingCart
> cart) throws CartItemModifyException {
>          // set basePrice using the calculateProductPrice service
>          if (_product != null && isModifiedPrice == false) {
>
>
>
Reply | Threaded
Open this post in threaded view
|

Re: svn commit: r1838320 - in /ofbiz/ofbiz-framework/trunk/applications: datamodel/data/seed/OrderSeedData.xml datamodel/data/seed/ProductSeedData.xml order/src/main/java/org/apache/ofbiz/order/shoppingcart/ShoppingCartItem.java

Suraj Khurana
Hi Pierre,

This is not a new patch, this is updated version of two years old patch
which has been already reviewed if you follow comments on ticket.
We need to add updated patch as well since many file path have been changed
and we have data files refactoring as well.

HTH.
--
Best Regards,
Suraj Khurana | Omni-channel OMS Technical Expert
HotWax Commerce  by  HotWax Systems

On Sat, Aug 18, 2018 at 2:13 PM, Pierre Smits <[hidden email]>
wrote:

> Hi Suraj,
>
> Please revert! Within 10 minutes you posted a new patch and committed it to
> trunk and closed the issue. It is customary to follow the 72 hr delay rule
> to allow the community to review the changes and assess the impact.
>
>
> Best regards,
>
> Pierre Smits
>
> Apache Trafodion <https://trafodion.apache.org>, Vice President
> Apache Directory <https://directory.apache.org>, PMC Member
> Apache Incubator <https://incubator.apache.org>, committer
> Apache OFBiz <https://ofbiz.apache.org>, contributor since 2008
> Apache Steve <https://steve.apache.org>, committer
>
> On Sat, Aug 18, 2018 at 10:34 AM, <[hidden email]> wrote:
>
> > Author: surajk
> > Date: Sat Aug 18 08:34:18 2018
> > New Revision: 1838320
> >
> > URL: http://svn.apache.org/viewvc?rev=1838320&view=rev
> > Log:
> > Improved: Added support to calculate deposit price as well while creating
> > shopping cart item.
> > (OFBIZ-7482)
> >
> > Modified:
> >     ofbiz/ofbiz-framework/trunk/applications/datamodel/data/
> > seed/OrderSeedData.xml
> >     ofbiz/ofbiz-framework/trunk/applications/datamodel/data/
> > seed/ProductSeedData.xml
> >     ofbiz/ofbiz-framework/trunk/applications/order/src/main/
> > java/org/apache/ofbiz/order/shoppingcart/ShoppingCartItem.java
> >
> > Modified: ofbiz/ofbiz-framework/trunk/applications/datamodel/data/
> > seed/OrderSeedData.xml
> > URL: http://svn.apache.org/viewvc/ofbiz/ofbiz-framework/trunk/
> > applications/datamodel/data/seed/OrderSeedData.xml?rev=
> > 1838320&r1=1838319&r2=1838320&view=diff
> > ============================================================
> > ==================
> > --- ofbiz/ofbiz-framework/trunk/applications/datamodel/data/
> seed/OrderSeedData.xml
> > (original)
> > +++ ofbiz/ofbiz-framework/trunk/applications/datamodel/data/
> seed/OrderSeedData.xml
> > Sat Aug 18 08:34:18 2018
> > @@ -49,6 +49,7 @@ under the License.
> >      <OrderAdjustmentType description="Additional Feature" hasTable="N"
> > orderAdjustmentTypeId="ADDITIONAL_FEATURE"/>
> >      <OrderAdjustmentType description="Warranty" hasTable="N"
> > orderAdjustmentTypeId="WARRANTY_ADJUSTMENT"/>
> >      <OrderAdjustmentType description="Marketing Package Adjustment"
> > hasTable="N" orderAdjustmentTypeId="MKTG_PKG_AUTO_ADJUST"/>
> > +    <OrderAdjustmentType description="Deposit" hasTable="N"
> > orderAdjustmentTypeId="DEPOSIT_ADJUSTMENT"/>
> >
> >      <OrderBlacklistType orderBlacklistTypeId="BLACKLIST_ADDRESS"
> > description="Addresss"/>
> >      <OrderBlacklistType orderBlacklistTypeId="BLACKLIST_CREDITCARD"
> > description="Credit Card"/>
> >
> > Modified: ofbiz/ofbiz-framework/trunk/applications/datamodel/data/
> > seed/ProductSeedData.xml
> > URL: http://svn.apache.org/viewvc/ofbiz/ofbiz-framework/trunk/
> > applications/datamodel/data/seed/ProductSeedData.xml?rev=
> > 1838320&r1=1838319&r2=1838320&view=diff
> > ============================================================
> > ==================
> > --- ofbiz/ofbiz-framework/trunk/applications/datamodel/data/
> seed/ProductSeedData.xml
> > (original)
> > +++ ofbiz/ofbiz-framework/trunk/applications/datamodel/data/
> seed/ProductSeedData.xml
> > Sat Aug 18 08:34:18 2018
> > @@ -281,6 +281,7 @@ under the License.
> >      <ProductPriceType description="Minimum Order Price"
> > productPriceTypeId="MINIMUM_ORDER_PRICE"/>
> >      <ProductPriceType description="Shipping Allowance Price"
> > productPriceTypeId="SHIPPING_ALLOWANCE"/>
> >
> > +    <ProductPricePurpose description="Deposit price"
> > productPricePurposeId="DEPOSIT"/>
> >      <ProductPricePurpose description="Purchase/Initial"
> > productPricePurposeId="PURCHASE"/>
> >      <ProductPricePurpose description="Recurring Charge"
> > productPricePurposeId="RECURRING_CHARGE"/>
> >      <ProductPricePurpose description="Usage Charge"
> > productPricePurposeId="USAGE_CHARGE"/>
> >
> > Modified: ofbiz/ofbiz-framework/trunk/applications/order/src/main/
> > java/org/apache/ofbiz/order/shoppingcart/ShoppingCartItem.java
> > URL: http://svn.apache.org/viewvc/ofbiz/ofbiz-framework/trunk/
> > applications/order/src/main/java/org/apache/ofbiz/order/
> > shoppingcart/ShoppingCartItem.java?rev=1838320&r1=1838319&
> > r2=1838320&view=diff
> > ============================================================
> > ==================
> > --- ofbiz/ofbiz-framework/trunk/applications/order/src/main/
> > java/org/apache/ofbiz/order/shoppingcart/ShoppingCartItem.java
> (original)
> > +++ ofbiz/ofbiz-framework/trunk/applications/order/src/main/
> > java/org/apache/ofbiz/order/shoppingcart/ShoppingCartItem.java Sat Aug
> 18
> > 08:34:18 2018
> > @@ -1038,6 +1038,7 @@ public class ShoppingCartItem implements
> >              ProductPromoWorker.doPromotions(cart, dispatcher);
> >          }
> >
> > +        calcDepositAdjustments();
> >          if (!"PURCHASE_ORDER".equals(cart.getOrderType())) {
> >              // store the auto-save cart
> >              if (triggerExternalOps && ProductStoreWorker.
> autoSaveCart(delegator,
> > productStoreId)) {
> > @@ -1061,6 +1062,33 @@ public class ShoppingCartItem implements
> >          }
> >      }
> >
> > +    public void calcDepositAdjustments() {
> > +        List<GenericValue>itemAdjustments = this.getAdjustments();
> > +        try {
> > +            GenericValue depositAmount = EntityQuery.use(delegator).
> > from("ProductPrice").where("productId", this.getProductId(),
> > "productPricePurposeId", "DEPOSIT", "productPriceTypeId",
> > "DEFAULT_PRICE").filterByDate().queryFirst();
> > +            if (UtilValidate.isNotEmpty(depositAmount)) {
> > +                Boolean updatedDepositAmount = false;
> > +                BigDecimal adjustmentAmount =
> > depositAmount.getBigDecimal("price").multiply(this.getQuantity(),
> > generalRounding);
> > +                // itemAdjustments is a reference so directly setting
> > updated amount to the same.
> > +                    for(GenericValue itemAdjustment : itemAdjustments) {
> > +                    if("DEPOSIT_ADJUSTMENT".equals(itemAdjustment.
> > getString("orderAdjustmentTypeId"))) {
> > +                            itemAdjustment.set("amount",
> > adjustmentAmount);
> > +                            updatedDepositAmount = true;
> > +                        }
> > +                }
> > +                if (!updatedDepositAmount) {
> > +                    GenericValue orderAdjustment = delegator.makeValue("
> > OrderAdjustment");
> > +                    orderAdjustment.set("orderAdjustmentTypeId",
> > "DEPOSIT_ADJUSTMENT");
> > +                    orderAdjustment.set("description", "Surcharge
> > Adjustment");
> > +                    orderAdjustment.set("amount", adjustmentAmount);
> > +                    this.addAdjustment(orderAdjustment);
> > +                }
> > +            }
> > +        } catch (GenericEntityException e){
> > +            Debug.logError("Error in fetching deposite price details!!",
> > module);
> > +        }
> > +    }
> > +
> >      public void updatePrice(LocalDispatcher dispatcher, ShoppingCart
> > cart) throws CartItemModifyException {
> >          // set basePrice using the calculateProductPrice service
> >          if (_product != null && isModifiedPrice == false) {
> >
> >
> >
>
Reply | Threaded
Open this post in threaded view
|

Re: svn commit: r1838320 - in /ofbiz/ofbiz-framework/trunk/applications: datamodel/data/seed/OrderSeedData.xml datamodel/data/seed/ProductSeedData.xml order/src/main/java/org/apache/ofbiz/order/shoppingcart/ShoppingCartItem.java

Pierre Smits-3
As Michael recently pointed out in another thread:

{quote}

*If it does break anything or introduces functionality which is not working
completely, we should revert.*

{quote}

And:

{quote}

*We are struggling with half baked, incomplete or buggy code in several
areas which often shows up a long time after it was committed. In other
cases, even if problems are raised soon after the commit, the code is left
untouched for a long time until it is work on again or simply is forgotten.*

{quote}


Best regards,

Pierre Smits

Apache Trafodion <https://trafodion.apache.org>, Vice President
Apache Directory <https://directory.apache.org>, PMC Member
Apache Incubator <https://incubator.apache.org>, committer
*Apache OFBiz <https://ofbiz.apache.org>, contributor (without privileges)
since 2008*
Apache Steve <https://steve.apache.org>, committer

On Sat, Aug 18, 2018 at 10:52 AM, Suraj Khurana <
[hidden email]> wrote:

> Hi Pierre,
>
> This is not a new patch, this is updated version of two years old patch
> which has been already reviewed if you follow comments on ticket.
> We need to add updated patch as well since many file path have been changed
> and we have data files refactoring as well.
>
> HTH.
> --
> Best Regards,
> Suraj Khurana | Omni-channel OMS Technical Expert
> HotWax Commerce  by  HotWax Systems
>
> On Sat, Aug 18, 2018 at 2:13 PM, Pierre Smits <[hidden email]>
> wrote:
>
> > Hi Suraj,
> >
> > Please revert! Within 10 minutes you posted a new patch and committed it
> to
> > trunk and closed the issue. It is customary to follow the 72 hr delay
> rule
> > to allow the community to review the changes and assess the impact.
> >
> >
> > Best regards,
> >
> > Pierre Smits
> >
> > Apache Trafodion <https://trafodion.apache.org>, Vice President
> > Apache Directory <https://directory.apache.org>, PMC Member
> > Apache Incubator <https://incubator.apache.org>, committer
> > Apache OFBiz <https://ofbiz.apache.org>, contributor since 2008
> > Apache Steve <https://steve.apache.org>, committer
> >
> > On Sat, Aug 18, 2018 at 10:34 AM, <[hidden email]> wrote:
> >
> > > Author: surajk
> > > Date: Sat Aug 18 08:34:18 2018
> > > New Revision: 1838320
> > >
> > > URL: http://svn.apache.org/viewvc?rev=1838320&view=rev
> > > Log:
> > > Improved: Added support to calculate deposit price as well while
> creating
> > > shopping cart item.
> > > (OFBIZ-7482)
> > >
> > > Modified:
> > >     ofbiz/ofbiz-framework/trunk/applications/datamodel/data/
> > > seed/OrderSeedData.xml
> > >     ofbiz/ofbiz-framework/trunk/applications/datamodel/data/
> > > seed/ProductSeedData.xml
> > >     ofbiz/ofbiz-framework/trunk/applications/order/src/main/
> > > java/org/apache/ofbiz/order/shoppingcart/ShoppingCartItem.java
> > >
> > > Modified: ofbiz/ofbiz-framework/trunk/applications/datamodel/data/
> > > seed/OrderSeedData.xml
> > > URL: http://svn.apache.org/viewvc/ofbiz/ofbiz-framework/trunk/
> > > applications/datamodel/data/seed/OrderSeedData.xml?rev=
> > > 1838320&r1=1838319&r2=1838320&view=diff
> > > ============================================================
> > > ==================
> > > --- ofbiz/ofbiz-framework/trunk/applications/datamodel/data/
> > seed/OrderSeedData.xml
> > > (original)
> > > +++ ofbiz/ofbiz-framework/trunk/applications/datamodel/data/
> > seed/OrderSeedData.xml
> > > Sat Aug 18 08:34:18 2018
> > > @@ -49,6 +49,7 @@ under the License.
> > >      <OrderAdjustmentType description="Additional Feature" hasTable="N"
> > > orderAdjustmentTypeId="ADDITIONAL_FEATURE"/>
> > >      <OrderAdjustmentType description="Warranty" hasTable="N"
> > > orderAdjustmentTypeId="WARRANTY_ADJUSTMENT"/>
> > >      <OrderAdjustmentType description="Marketing Package Adjustment"
> > > hasTable="N" orderAdjustmentTypeId="MKTG_PKG_AUTO_ADJUST"/>
> > > +    <OrderAdjustmentType description="Deposit" hasTable="N"
> > > orderAdjustmentTypeId="DEPOSIT_ADJUSTMENT"/>
> > >
> > >      <OrderBlacklistType orderBlacklistTypeId="BLACKLIST_ADDRESS"
> > > description="Addresss"/>
> > >      <OrderBlacklistType orderBlacklistTypeId="BLACKLIST_CREDITCARD"
> > > description="Credit Card"/>
> > >
> > > Modified: ofbiz/ofbiz-framework/trunk/applications/datamodel/data/
> > > seed/ProductSeedData.xml
> > > URL: http://svn.apache.org/viewvc/ofbiz/ofbiz-framework/trunk/
> > > applications/datamodel/data/seed/ProductSeedData.xml?rev=
> > > 1838320&r1=1838319&r2=1838320&view=diff
> > > ============================================================
> > > ==================
> > > --- ofbiz/ofbiz-framework/trunk/applications/datamodel/data/
> > seed/ProductSeedData.xml
> > > (original)
> > > +++ ofbiz/ofbiz-framework/trunk/applications/datamodel/data/
> > seed/ProductSeedData.xml
> > > Sat Aug 18 08:34:18 2018
> > > @@ -281,6 +281,7 @@ under the License.
> > >      <ProductPriceType description="Minimum Order Price"
> > > productPriceTypeId="MINIMUM_ORDER_PRICE"/>
> > >      <ProductPriceType description="Shipping Allowance Price"
> > > productPriceTypeId="SHIPPING_ALLOWANCE"/>
> > >
> > > +    <ProductPricePurpose description="Deposit price"
> > > productPricePurposeId="DEPOSIT"/>
> > >      <ProductPricePurpose description="Purchase/Initial"
> > > productPricePurposeId="PURCHASE"/>
> > >      <ProductPricePurpose description="Recurring Charge"
> > > productPricePurposeId="RECURRING_CHARGE"/>
> > >      <ProductPricePurpose description="Usage Charge"
> > > productPricePurposeId="USAGE_CHARGE"/>
> > >
> > > Modified: ofbiz/ofbiz-framework/trunk/applications/order/src/main/
> > > java/org/apache/ofbiz/order/shoppingcart/ShoppingCartItem.java
> > > URL: http://svn.apache.org/viewvc/ofbiz/ofbiz-framework/trunk/
> > > applications/order/src/main/java/org/apache/ofbiz/order/
> > > shoppingcart/ShoppingCartItem.java?rev=1838320&r1=1838319&
> > > r2=1838320&view=diff
> > > ============================================================
> > > ==================
> > > --- ofbiz/ofbiz-framework/trunk/applications/order/src/main/
> > > java/org/apache/ofbiz/order/shoppingcart/ShoppingCartItem.java
> > (original)
> > > +++ ofbiz/ofbiz-framework/trunk/applications/order/src/main/
> > > java/org/apache/ofbiz/order/shoppingcart/ShoppingCartItem.java Sat Aug
> > 18
> > > 08:34:18 2018
> > > @@ -1038,6 +1038,7 @@ public class ShoppingCartItem implements
> > >              ProductPromoWorker.doPromotions(cart, dispatcher);
> > >          }
> > >
> > > +        calcDepositAdjustments();
> > >          if (!"PURCHASE_ORDER".equals(cart.getOrderType())) {
> > >              // store the auto-save cart
> > >              if (triggerExternalOps && ProductStoreWorker.
> > autoSaveCart(delegator,
> > > productStoreId)) {
> > > @@ -1061,6 +1062,33 @@ public class ShoppingCartItem implements
> > >          }
> > >      }
> > >
> > > +    public void calcDepositAdjustments() {
> > > +        List<GenericValue>itemAdjustments = this.getAdjustments();
> > > +        try {
> > > +            GenericValue depositAmount = EntityQuery.use(delegator).
> > > from("ProductPrice").where("productId", this.getProductId(),
> > > "productPricePurposeId", "DEPOSIT", "productPriceTypeId",
> > > "DEFAULT_PRICE").filterByDate().queryFirst();
> > > +            if (UtilValidate.isNotEmpty(depositAmount)) {
> > > +                Boolean updatedDepositAmount = false;
> > > +                BigDecimal adjustmentAmount =
> > > depositAmount.getBigDecimal("price").multiply(this.getQuantity(),
> > > generalRounding);
> > > +                // itemAdjustments is a reference so directly setting
> > > updated amount to the same.
> > > +                    for(GenericValue itemAdjustment :
> itemAdjustments) {
> > > +                    if("DEPOSIT_ADJUSTMENT".equals(itemAdjustment.
> > > getString("orderAdjustmentTypeId"))) {
> > > +                            itemAdjustment.set("amount",
> > > adjustmentAmount);
> > > +                            updatedDepositAmount = true;
> > > +                        }
> > > +                }
> > > +                if (!updatedDepositAmount) {
> > > +                    GenericValue orderAdjustment =
> delegator.makeValue("
> > > OrderAdjustment");
> > > +                    orderAdjustment.set("orderAdjustmentTypeId",
> > > "DEPOSIT_ADJUSTMENT");
> > > +                    orderAdjustment.set("description", "Surcharge
> > > Adjustment");
> > > +                    orderAdjustment.set("amount", adjustmentAmount);
> > > +                    this.addAdjustment(orderAdjustment);
> > > +                }
> > > +            }
> > > +        } catch (GenericEntityException e){
> > > +            Debug.logError("Error in fetching deposite price
> details!!",
> > > module);
> > > +        }
> > > +    }
> > > +
> > >      public void updatePrice(LocalDispatcher dispatcher, ShoppingCart
> > > cart) throws CartItemModifyException {
> > >          // set basePrice using the calculateProductPrice service
> > >          if (_product != null && isModifiedPrice == false) {
> > >
> > >
> > >
> >
>
Reply | Threaded
Open this post in threaded view
|

Re: svn commit: r1838320 - in /ofbiz/ofbiz-framework/trunk/applications: datamodel/data/seed/OrderSeedData.xml datamodel/data/seed/ProductSeedData.xml order/src/main/java/org/apache/ofbiz/order/shoppingcart/ShoppingCartItem.java

Michael Brohl-3
How do these citations relate to Suraj‘s work?

Please provide arguments in your own words if you think someone‘s work has flaws and should be reverted.

Thanks,
Michael


> Am 18.08.2018 um 13:54 schrieb Pierre Smits <[hidden email]>:
>
> As Michael recently pointed out in another thread:
>
> {quote}
>
> *If it does break anything or introduces functionality which is not working
> completely, we should revert.*
>
> {quote}
>
> And:
>
> {quote}
>
> *We are struggling with half baked, incomplete or buggy code in several
> areas which often shows up a long time after it was committed. In other
> cases, even if problems are raised soon after the commit, the code is left
> untouched for a long time until it is work on again or simply is forgotten.*
>
> {quote}
>
>
> Best regards,
>
> Pierre Smits
>
> On Sat, Aug 18, 2018 at 10:52 AM, Suraj Khurana <
> [hidden email]> wrote:
>
>> Hi Pierre,
>>
>> This is not a new patch, this is updated version of two years old patch
>> which has been already reviewed if you follow comments on ticket.
>> We need to add updated patch as well since many file path have been changed
>> and we have data files refactoring as well.
>>
>> HTH.
>> --
>> Best Regards,
>> Suraj Khurana | Omni-channel OMS Technical Expert
>> HotWax Commerce  by  HotWax Systems
>>
>> On Sat, Aug 18, 2018 at 2:13 PM, Pierre Smits <[hidden email]>
>> wrote:
>>
>>> Hi Suraj,
>>>
>>> Please revert! Within 10 minutes you posted a new patch and committed it
>> to
>>> trunk and closed the issue. It is customary to follow the 72 hr delay
>> rule
>>> to allow the community to review the changes and assess the impact.
>>>
>>>
>>> Best regards,
>>>
>>> Pierre Smits
>>>
>>> Apache Trafodion <https://trafodion.apache.org>, Vice President
>>> Apache Directory <https://directory.apache.org>, PMC Member
>>> Apache Incubator <https://incubator.apache.org>, committer
>>> Apache OFBiz <https://ofbiz.apache.org>, contributor since 2008
>>> Apache Steve <https://steve.apache.org>, committer
>>>
>>>> On Sat, Aug 18, 2018 at 10:34 AM, <[hidden email]> wrote:
>>>>
>>>> Author: surajk
>>>> Date: Sat Aug 18 08:34:18 2018
>>>> New Revision: 1838320
>>>>
>>>> URL: http://svn.apache.org/viewvc?rev=1838320&view=rev
>>>> Log:
>>>> Improved: Added support to calculate deposit price as well while
>> creating
>>>> shopping cart item.
>>>> (OFBIZ-7482)
>>>>
>>>> Modified:
>>>>    ofbiz/ofbiz-framework/trunk/applications/datamodel/data/
>>>> seed/OrderSeedData.xml
>>>>    ofbiz/ofbiz-framework/trunk/applications/datamodel/data/
>>>> seed/ProductSeedData.xml
>>>>    ofbiz/ofbiz-framework/trunk/applications/order/src/main/
>>>> java/org/apache/ofbiz/order/shoppingcart/ShoppingCartItem.java
>>>>
>>>> Modified: ofbiz/ofbiz-framework/trunk/applications/datamodel/data/
>>>> seed/OrderSeedData.xml
>>>> URL: http://svn.apache.org/viewvc/ofbiz/ofbiz-framework/trunk/
>>>> applications/datamodel/data/seed/OrderSeedData.xml?rev=
>>>> 1838320&r1=1838319&r2=1838320&view=diff
>>>> ============================================================
>>>> ==================
>>>> --- ofbiz/ofbiz-framework/trunk/applications/datamodel/data/
>>> seed/OrderSeedData.xml
>>>> (original)
>>>> +++ ofbiz/ofbiz-framework/trunk/applications/datamodel/data/
>>> seed/OrderSeedData.xml
>>>> Sat Aug 18 08:34:18 2018
>>>> @@ -49,6 +49,7 @@ under the License.
>>>>     <OrderAdjustmentType description="Additional Feature" hasTable="N"
>>>> orderAdjustmentTypeId="ADDITIONAL_FEATURE"/>
>>>>     <OrderAdjustmentType description="Warranty" hasTable="N"
>>>> orderAdjustmentTypeId="WARRANTY_ADJUSTMENT"/>
>>>>     <OrderAdjustmentType description="Marketing Package Adjustment"
>>>> hasTable="N" orderAdjustmentTypeId="MKTG_PKG_AUTO_ADJUST"/>
>>>> +    <OrderAdjustmentType description="Deposit" hasTable="N"
>>>> orderAdjustmentTypeId="DEPOSIT_ADJUSTMENT"/>
>>>>
>>>>     <OrderBlacklistType orderBlacklistTypeId="BLACKLIST_ADDRESS"
>>>> description="Addresss"/>
>>>>     <OrderBlacklistType orderBlacklistTypeId="BLACKLIST_CREDITCARD"
>>>> description="Credit Card"/>
>>>>
>>>> Modified: ofbiz/ofbiz-framework/trunk/applications/datamodel/data/
>>>> seed/ProductSeedData.xml
>>>> URL: http://svn.apache.org/viewvc/ofbiz/ofbiz-framework/trunk/
>>>> applications/datamodel/data/seed/ProductSeedData.xml?rev=
>>>> 1838320&r1=1838319&r2=1838320&view=diff
>>>> ============================================================
>>>> ==================
>>>> --- ofbiz/ofbiz-framework/trunk/applications/datamodel/data/
>>> seed/ProductSeedData.xml
>>>> (original)
>>>> +++ ofbiz/ofbiz-framework/trunk/applications/datamodel/data/
>>> seed/ProductSeedData.xml
>>>> Sat Aug 18 08:34:18 2018
>>>> @@ -281,6 +281,7 @@ under the License.
>>>>     <ProductPriceType description="Minimum Order Price"
>>>> productPriceTypeId="MINIMUM_ORDER_PRICE"/>
>>>>     <ProductPriceType description="Shipping Allowance Price"
>>>> productPriceTypeId="SHIPPING_ALLOWANCE"/>
>>>>
>>>> +    <ProductPricePurpose description="Deposit price"
>>>> productPricePurposeId="DEPOSIT"/>
>>>>     <ProductPricePurpose description="Purchase/Initial"
>>>> productPricePurposeId="PURCHASE"/>
>>>>     <ProductPricePurpose description="Recurring Charge"
>>>> productPricePurposeId="RECURRING_CHARGE"/>
>>>>     <ProductPricePurpose description="Usage Charge"
>>>> productPricePurposeId="USAGE_CHARGE"/>
>>>>
>>>> Modified: ofbiz/ofbiz-framework/trunk/applications/order/src/main/
>>>> java/org/apache/ofbiz/order/shoppingcart/ShoppingCartItem.java
>>>> URL: http://svn.apache.org/viewvc/ofbiz/ofbiz-framework/trunk/
>>>> applications/order/src/main/java/org/apache/ofbiz/order/
>>>> shoppingcart/ShoppingCartItem.java?rev=1838320&r1=1838319&
>>>> r2=1838320&view=diff
>>>> ============================================================
>>>> ==================
>>>> --- ofbiz/ofbiz-framework/trunk/applications/order/src/main/
>>>> java/org/apache/ofbiz/order/shoppingcart/ShoppingCartItem.java
>>> (original)
>>>> +++ ofbiz/ofbiz-framework/trunk/applications/order/src/main/
>>>> java/org/apache/ofbiz/order/shoppingcart/ShoppingCartItem.java Sat Aug
>>> 18
>>>> 08:34:18 2018
>>>> @@ -1038,6 +1038,7 @@ public class ShoppingCartItem implements
>>>>             ProductPromoWorker.doPromotions(cart, dispatcher);
>>>>         }
>>>>
>>>> +        calcDepositAdjustments();
>>>>         if (!"PURCHASE_ORDER".equals(cart.getOrderType())) {
>>>>             // store the auto-save cart
>>>>             if (triggerExternalOps && ProductStoreWorker.
>>> autoSaveCart(delegator,
>>>> productStoreId)) {
>>>> @@ -1061,6 +1062,33 @@ public class ShoppingCartItem implements
>>>>         }
>>>>     }
>>>>
>>>> +    public void calcDepositAdjustments() {
>>>> +        List<GenericValue>itemAdjustments = this.getAdjustments();
>>>> +        try {
>>>> +            GenericValue depositAmount = EntityQuery.use(delegator).
>>>> from("ProductPrice").where("productId", this.getProductId(),
>>>> "productPricePurposeId", "DEPOSIT", "productPriceTypeId",
>>>> "DEFAULT_PRICE").filterByDate().queryFirst();
>>>> +            if (UtilValidate.isNotEmpty(depositAmount)) {
>>>> +                Boolean updatedDepositAmount = false;
>>>> +                BigDecimal adjustmentAmount =
>>>> depositAmount.getBigDecimal("price").multiply(this.getQuantity(),
>>>> generalRounding);
>>>> +                // itemAdjustments is a reference so directly setting
>>>> updated amount to the same.
>>>> +                    for(GenericValue itemAdjustment :
>> itemAdjustments) {
>>>> +                    if("DEPOSIT_ADJUSTMENT".equals(itemAdjustment.
>>>> getString("orderAdjustmentTypeId"))) {
>>>> +                            itemAdjustment.set("amount",
>>>> adjustmentAmount);
>>>> +                            updatedDepositAmount = true;
>>>> +                        }
>>>> +                }
>>>> +                if (!updatedDepositAmount) {
>>>> +                    GenericValue orderAdjustment =
>> delegator.makeValue("
>>>> OrderAdjustment");
>>>> +                    orderAdjustment.set("orderAdjustmentTypeId",
>>>> "DEPOSIT_ADJUSTMENT");
>>>> +                    orderAdjustment.set("description", "Surcharge
>>>> Adjustment");
>>>> +                    orderAdjustment.set("amount", adjustmentAmount);
>>>> +                    this.addAdjustment(orderAdjustment);
>>>> +                }
>>>> +            }
>>>> +        } catch (GenericEntityException e){
>>>> +            Debug.logError("Error in fetching deposite price
>> details!!",
>>>> module);
>>>> +        }
>>>> +    }
>>>> +
>>>>     public void updatePrice(LocalDispatcher dispatcher, ShoppingCart
>>>> cart) throws CartItemModifyException {
>>>>         // set basePrice using the calculateProductPrice service
>>>>         if (_product != null && isModifiedPrice == false) {
>>>>
>>>>
>>>>
>>>
>>
Reply | Threaded
Open this post in threaded view
|

Re: svn commit: r1838320 - in /ofbiz/ofbiz-framework/trunk/applications: datamodel/data/seed/OrderSeedData.xml datamodel/data/seed/ProductSeedData.xml order/src/main/java/org/apache/ofbiz/order/shoppingcart/ShoppingCartItem.java

Pierre Smits-3
Please read the comment in the related ticket [1]

https://issues.apache.org/jira/browse/OFBIZ-7482?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=16584721#comment-16584721


Best regards,

Pierre Smits

Apache Trafodion <https://trafodion.apache.org>, Vice President
Apache Directory <https://directory.apache.org>, PMC Member
Apache Incubator <https://incubator.apache.org>, committer
*Apache OFBiz <https://ofbiz.apache.org>, contributor (without privileges)
since 2008*
Apache Steve <https://steve.apache.org>, committer

On Sat, Aug 18, 2018 at 9:01 PM, Michael Brohl <[hidden email]>
wrote:

> How do these citations relate to Suraj‘s work?
>
> Please provide arguments in your own words if you think someone‘s work has
> flaws and should be reverted.
>
> Thanks,
> Michael
>
>
> > Am 18.08.2018 um 13:54 schrieb Pierre Smits <[hidden email]>:
> >
> > As Michael recently pointed out in another thread:
> >
> > {quote}
> >
> > *If it does break anything or introduces functionality which is not
> working
> > completely, we should revert.*
> >
> > {quote}
> >
> > And:
> >
> > {quote}
> >
> > *We are struggling with half baked, incomplete or buggy code in several
> > areas which often shows up a long time after it was committed. In other
> > cases, even if problems are raised soon after the commit, the code is
> left
> > untouched for a long time until it is work on again or simply is
> forgotten.*
> >
> > {quote}
> >
> >
> > Best regards,
> >
> > Pierre Smits
> >
> > On Sat, Aug 18, 2018 at 10:52 AM, Suraj Khurana <
> > [hidden email]> wrote:
> >
> >> Hi Pierre,
> >>
> >> This is not a new patch, this is updated version of two years old patch
> >> which has been already reviewed if you follow comments on ticket.
> >> We need to add updated patch as well since many file path have been
> changed
> >> and we have data files refactoring as well.
> >>
> >> HTH.
> >> --
> >> Best Regards,
> >> Suraj Khurana | Omni-channel OMS Technical Expert
> >> HotWax Commerce  by  HotWax Systems
> >>
> >> On Sat, Aug 18, 2018 at 2:13 PM, Pierre Smits <[hidden email]>
> >> wrote:
> >>
> >>> Hi Suraj,
> >>>
> >>> Please revert! Within 10 minutes you posted a new patch and committed
> it
> >> to
> >>> trunk and closed the issue. It is customary to follow the 72 hr delay
> >> rule
> >>> to allow the community to review the changes and assess the impact.
> >>>
> >>>
> >>> Best regards,
> >>>
> >>> Pierre Smits
> >>>
> >>> Apache Trafodion <https://trafodion.apache.org>, Vice President
> >>> Apache Directory <https://directory.apache.org>, PMC Member
> >>> Apache Incubator <https://incubator.apache.org>, committer
> >>> Apache OFBiz <https://ofbiz.apache.org>, contributor since 2008
> >>> Apache Steve <https://steve.apache.org>, committer
> >>>
> >>>> On Sat, Aug 18, 2018 at 10:34 AM, <[hidden email]> wrote:
> >>>>
> >>>> Author: surajk
> >>>> Date: Sat Aug 18 08:34:18 2018
> >>>> New Revision: 1838320
> >>>>
> >>>> URL: http://svn.apache.org/viewvc?rev=1838320&view=rev
> >>>> Log:
> >>>> Improved: Added support to calculate deposit price as well while
> >> creating
> >>>> shopping cart item.
> >>>> (OFBIZ-7482)
> >>>>
> >>>> Modified:
> >>>>    ofbiz/ofbiz-framework/trunk/applications/datamodel/data/
> >>>> seed/OrderSeedData.xml
> >>>>    ofbiz/ofbiz-framework/trunk/applications/datamodel/data/
> >>>> seed/ProductSeedData.xml
> >>>>    ofbiz/ofbiz-framework/trunk/applications/order/src/main/
> >>>> java/org/apache/ofbiz/order/shoppingcart/ShoppingCartItem.java
> >>>>
> >>>> Modified: ofbiz/ofbiz-framework/trunk/applications/datamodel/data/
> >>>> seed/OrderSeedData.xml
> >>>> URL: http://svn.apache.org/viewvc/ofbiz/ofbiz-framework/trunk/
> >>>> applications/datamodel/data/seed/OrderSeedData.xml?rev=
> >>>> 1838320&r1=1838319&r2=1838320&view=diff
> >>>> ============================================================
> >>>> ==================
> >>>> --- ofbiz/ofbiz-framework/trunk/applications/datamodel/data/
> >>> seed/OrderSeedData.xml
> >>>> (original)
> >>>> +++ ofbiz/ofbiz-framework/trunk/applications/datamodel/data/
> >>> seed/OrderSeedData.xml
> >>>> Sat Aug 18 08:34:18 2018
> >>>> @@ -49,6 +49,7 @@ under the License.
> >>>>     <OrderAdjustmentType description="Additional Feature" hasTable="N"
> >>>> orderAdjustmentTypeId="ADDITIONAL_FEATURE"/>
> >>>>     <OrderAdjustmentType description="Warranty" hasTable="N"
> >>>> orderAdjustmentTypeId="WARRANTY_ADJUSTMENT"/>
> >>>>     <OrderAdjustmentType description="Marketing Package Adjustment"
> >>>> hasTable="N" orderAdjustmentTypeId="MKTG_PKG_AUTO_ADJUST"/>
> >>>> +    <OrderAdjustmentType description="Deposit" hasTable="N"
> >>>> orderAdjustmentTypeId="DEPOSIT_ADJUSTMENT"/>
> >>>>
> >>>>     <OrderBlacklistType orderBlacklistTypeId="BLACKLIST_ADDRESS"
> >>>> description="Addresss"/>
> >>>>     <OrderBlacklistType orderBlacklistTypeId="BLACKLIST_CREDITCARD"
> >>>> description="Credit Card"/>
> >>>>
> >>>> Modified: ofbiz/ofbiz-framework/trunk/applications/datamodel/data/
> >>>> seed/ProductSeedData.xml
> >>>> URL: http://svn.apache.org/viewvc/ofbiz/ofbiz-framework/trunk/
> >>>> applications/datamodel/data/seed/ProductSeedData.xml?rev=
> >>>> 1838320&r1=1838319&r2=1838320&view=diff
> >>>> ============================================================
> >>>> ==================
> >>>> --- ofbiz/ofbiz-framework/trunk/applications/datamodel/data/
> >>> seed/ProductSeedData.xml
> >>>> (original)
> >>>> +++ ofbiz/ofbiz-framework/trunk/applications/datamodel/data/
> >>> seed/ProductSeedData.xml
> >>>> Sat Aug 18 08:34:18 2018
> >>>> @@ -281,6 +281,7 @@ under the License.
> >>>>     <ProductPriceType description="Minimum Order Price"
> >>>> productPriceTypeId="MINIMUM_ORDER_PRICE"/>
> >>>>     <ProductPriceType description="Shipping Allowance Price"
> >>>> productPriceTypeId="SHIPPING_ALLOWANCE"/>
> >>>>
> >>>> +    <ProductPricePurpose description="Deposit price"
> >>>> productPricePurposeId="DEPOSIT"/>
> >>>>     <ProductPricePurpose description="Purchase/Initial"
> >>>> productPricePurposeId="PURCHASE"/>
> >>>>     <ProductPricePurpose description="Recurring Charge"
> >>>> productPricePurposeId="RECURRING_CHARGE"/>
> >>>>     <ProductPricePurpose description="Usage Charge"
> >>>> productPricePurposeId="USAGE_CHARGE"/>
> >>>>
> >>>> Modified: ofbiz/ofbiz-framework/trunk/applications/order/src/main/
> >>>> java/org/apache/ofbiz/order/shoppingcart/ShoppingCartItem.java
> >>>> URL: http://svn.apache.org/viewvc/ofbiz/ofbiz-framework/trunk/
> >>>> applications/order/src/main/java/org/apache/ofbiz/order/
> >>>> shoppingcart/ShoppingCartItem.java?rev=1838320&r1=1838319&
> >>>> r2=1838320&view=diff
> >>>> ============================================================
> >>>> ==================
> >>>> --- ofbiz/ofbiz-framework/trunk/applications/order/src/main/
> >>>> java/org/apache/ofbiz/order/shoppingcart/ShoppingCartItem.java
> >>> (original)
> >>>> +++ ofbiz/ofbiz-framework/trunk/applications/order/src/main/
> >>>> java/org/apache/ofbiz/order/shoppingcart/ShoppingCartItem.java Sat
> Aug
> >>> 18
> >>>> 08:34:18 2018
> >>>> @@ -1038,6 +1038,7 @@ public class ShoppingCartItem implements
> >>>>             ProductPromoWorker.doPromotions(cart, dispatcher);
> >>>>         }
> >>>>
> >>>> +        calcDepositAdjustments();
> >>>>         if (!"PURCHASE_ORDER".equals(cart.getOrderType())) {
> >>>>             // store the auto-save cart
> >>>>             if (triggerExternalOps && ProductStoreWorker.
> >>> autoSaveCart(delegator,
> >>>> productStoreId)) {
> >>>> @@ -1061,6 +1062,33 @@ public class ShoppingCartItem implements
> >>>>         }
> >>>>     }
> >>>>
> >>>> +    public void calcDepositAdjustments() {
> >>>> +        List<GenericValue>itemAdjustments = this.getAdjustments();
> >>>> +        try {
> >>>> +            GenericValue depositAmount = EntityQuery.use(delegator).
> >>>> from("ProductPrice").where("productId", this.getProductId(),
> >>>> "productPricePurposeId", "DEPOSIT", "productPriceTypeId",
> >>>> "DEFAULT_PRICE").filterByDate().queryFirst();
> >>>> +            if (UtilValidate.isNotEmpty(depositAmount)) {
> >>>> +                Boolean updatedDepositAmount = false;
> >>>> +                BigDecimal adjustmentAmount =
> >>>> depositAmount.getBigDecimal("price").multiply(this.getQuantity(),
> >>>> generalRounding);
> >>>> +                // itemAdjustments is a reference so directly setting
> >>>> updated amount to the same.
> >>>> +                    for(GenericValue itemAdjustment :
> >> itemAdjustments) {
> >>>> +                    if("DEPOSIT_ADJUSTMENT".equals(itemAdjustment.
> >>>> getString("orderAdjustmentTypeId"))) {
> >>>> +                            itemAdjustment.set("amount",
> >>>> adjustmentAmount);
> >>>> +                            updatedDepositAmount = true;
> >>>> +                        }
> >>>> +                }
> >>>> +                if (!updatedDepositAmount) {
> >>>> +                    GenericValue orderAdjustment =
> >> delegator.makeValue("
> >>>> OrderAdjustment");
> >>>> +                    orderAdjustment.set("orderAdjustmentTypeId",
> >>>> "DEPOSIT_ADJUSTMENT");
> >>>> +                    orderAdjustment.set("description", "Surcharge
> >>>> Adjustment");
> >>>> +                    orderAdjustment.set("amount", adjustmentAmount);
> >>>> +                    this.addAdjustment(orderAdjustment);
> >>>> +                }
> >>>> +            }
> >>>> +        } catch (GenericEntityException e){
> >>>> +            Debug.logError("Error in fetching deposite price
> >> details!!",
> >>>> module);
> >>>> +        }
> >>>> +    }
> >>>> +
> >>>>     public void updatePrice(LocalDispatcher dispatcher, ShoppingCart
> >>>> cart) throws CartItemModifyException {
> >>>>         // set basePrice using the calculateProductPrice service
> >>>>         if (_product != null && isModifiedPrice == false) {
> >>>>
> >>>>
> >>>>
> >>>
> >>
>
Reply | Threaded
Open this post in threaded view
|

Re: svn commit: r1838320 - in /ofbiz/ofbiz-framework/trunk/applications: datamodel/data/seed/OrderSeedData.xml datamodel/data/seed/ProductSeedData.xml order/src/main/java/org/apache/ofbiz/order/shoppingcart/ShoppingCartItem.java

Michael Brohl-3
I have not the time to dig into the specific details right now so will
just give my thoughts on the process in general because of the citations:

1. we have to distinguish between (a) completely new functionality or
major refactorings and (b) the enhancement of functionality which is
already in the code base

2. for (a), we should first have consenus that we want the proposed
solution and we should look for a complete, well designed and carefully
tested solution before the first commit will be done. This is to prevent
the introduction of new problematic code.

3. for (b), I think every improvement of existing code/functionality
helps and should be committed if there are no flaws in the design or
technical solution and it does not break existing funtionality. of
course, it should be complete within the *scope* of the improvement.

4. if the solution for (b) does not cover other wishes or things which
could be enhanced also, this would be no reason to not commit the
improvement. If people have further requirements, they can provide
concepts and solutions/patches anytime to make things better.

In this case, for me it is important if Suraj's commit

a. breaks anything?

b. is vetoed by other committers in view of code quality or design flaws?

If none of these questions get a "yes", then I see no reason to revert.

If you have additional requirements, you are encouraged to provide
solutions or concepts for them.

Thanks,

Michael Brohl
ecomify GmbH
www.ecomify.de


Am 19.08.18 um 09:25 schrieb Pierre Smits:

> Please read the comment in the related ticket [1]
>
> https://issues.apache.org/jira/browse/OFBIZ-7482?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=16584721#comment-16584721
>
>
> Best regards,
>
> Pierre Smits
>
> Apache Trafodion <https://trafodion.apache.org>, Vice President
> Apache Directory <https://directory.apache.org>, PMC Member
> Apache Incubator <https://incubator.apache.org>, committer
> *Apache OFBiz <https://ofbiz.apache.org>, contributor (without privileges)
> since 2008*
> Apache Steve <https://steve.apache.org>, committer
>
> On Sat, Aug 18, 2018 at 9:01 PM, Michael Brohl <[hidden email]>
> wrote:
>
>> How do these citations relate to Suraj‘s work?
>>
>> Please provide arguments in your own words if you think someone‘s work has
>> flaws and should be reverted.
>>
>> Thanks,
>> Michael
>>
>>
>>> Am 18.08.2018 um 13:54 schrieb Pierre Smits <[hidden email]>:
>>>
>>> As Michael recently pointed out in another thread:
>>>
>>> {quote}
>>>
>>> *If it does break anything or introduces functionality which is not
>> working
>>> completely, we should revert.*
>>>
>>> {quote}
>>>
>>> And:
>>>
>>> {quote}
>>>
>>> *We are struggling with half baked, incomplete or buggy code in several
>>> areas which often shows up a long time after it was committed. In other
>>> cases, even if problems are raised soon after the commit, the code is
>> left
>>> untouched for a long time until it is work on again or simply is
>> forgotten.*
>>> {quote}
>>>
>>>
>>> Best regards,
>>>
>>> Pierre Smits
>>>
>>> On Sat, Aug 18, 2018 at 10:52 AM, Suraj Khurana <
>>> [hidden email]> wrote:
>>>
>>>> Hi Pierre,
>>>>
>>>> This is not a new patch, this is updated version of two years old patch
>>>> which has been already reviewed if you follow comments on ticket.
>>>> We need to add updated patch as well since many file path have been
>> changed
>>>> and we have data files refactoring as well.
>>>>
>>>> HTH.
>>>> --
>>>> Best Regards,
>>>> Suraj Khurana | Omni-channel OMS Technical Expert
>>>> HotWax Commerce  by  HotWax Systems
>>>>
>>>> On Sat, Aug 18, 2018 at 2:13 PM, Pierre Smits <[hidden email]>
>>>> wrote:
>>>>
>>>>> Hi Suraj,
>>>>>
>>>>> Please revert! Within 10 minutes you posted a new patch and committed
>> it
>>>> to
>>>>> trunk and closed the issue. It is customary to follow the 72 hr delay
>>>> rule
>>>>> to allow the community to review the changes and assess the impact.
>>>>>
>>>>>
>>>>> Best regards,
>>>>>
>>>>> Pierre Smits
>>>>>
>>>>> Apache Trafodion <https://trafodion.apache.org>, Vice President
>>>>> Apache Directory <https://directory.apache.org>, PMC Member
>>>>> Apache Incubator <https://incubator.apache.org>, committer
>>>>> Apache OFBiz <https://ofbiz.apache.org>, contributor since 2008
>>>>> Apache Steve <https://steve.apache.org>, committer
>>>>>
>>>>>> On Sat, Aug 18, 2018 at 10:34 AM, <[hidden email]> wrote:
>>>>>>
>>>>>> Author: surajk
>>>>>> Date: Sat Aug 18 08:34:18 2018
>>>>>> New Revision: 1838320
>>>>>>
>>>>>> URL: http://svn.apache.org/viewvc?rev=1838320&view=rev
>>>>>> Log:
>>>>>> Improved: Added support to calculate deposit price as well while
>>>> creating
>>>>>> shopping cart item.
>>>>>> (OFBIZ-7482)
>>>>>>
>>>>>> Modified:
>>>>>>     ofbiz/ofbiz-framework/trunk/applications/datamodel/data/
>>>>>> seed/OrderSeedData.xml
>>>>>>     ofbiz/ofbiz-framework/trunk/applications/datamodel/data/
>>>>>> seed/ProductSeedData.xml
>>>>>>     ofbiz/ofbiz-framework/trunk/applications/order/src/main/
>>>>>> java/org/apache/ofbiz/order/shoppingcart/ShoppingCartItem.java
>>>>>>
>>>>>> Modified: ofbiz/ofbiz-framework/trunk/applications/datamodel/data/
>>>>>> seed/OrderSeedData.xml
>>>>>> URL: http://svn.apache.org/viewvc/ofbiz/ofbiz-framework/trunk/
>>>>>> applications/datamodel/data/seed/OrderSeedData.xml?rev=
>>>>>> 1838320&r1=1838319&r2=1838320&view=diff
>>>>>> ============================================================
>>>>>> ==================
>>>>>> --- ofbiz/ofbiz-framework/trunk/applications/datamodel/data/
>>>>> seed/OrderSeedData.xml
>>>>>> (original)
>>>>>> +++ ofbiz/ofbiz-framework/trunk/applications/datamodel/data/
>>>>> seed/OrderSeedData.xml
>>>>>> Sat Aug 18 08:34:18 2018
>>>>>> @@ -49,6 +49,7 @@ under the License.
>>>>>>      <OrderAdjustmentType description="Additional Feature" hasTable="N"
>>>>>> orderAdjustmentTypeId="ADDITIONAL_FEATURE"/>
>>>>>>      <OrderAdjustmentType description="Warranty" hasTable="N"
>>>>>> orderAdjustmentTypeId="WARRANTY_ADJUSTMENT"/>
>>>>>>      <OrderAdjustmentType description="Marketing Package Adjustment"
>>>>>> hasTable="N" orderAdjustmentTypeId="MKTG_PKG_AUTO_ADJUST"/>
>>>>>> +    <OrderAdjustmentType description="Deposit" hasTable="N"
>>>>>> orderAdjustmentTypeId="DEPOSIT_ADJUSTMENT"/>
>>>>>>
>>>>>>      <OrderBlacklistType orderBlacklistTypeId="BLACKLIST_ADDRESS"
>>>>>> description="Addresss"/>
>>>>>>      <OrderBlacklistType orderBlacklistTypeId="BLACKLIST_CREDITCARD"
>>>>>> description="Credit Card"/>
>>>>>>
>>>>>> Modified: ofbiz/ofbiz-framework/trunk/applications/datamodel/data/
>>>>>> seed/ProductSeedData.xml
>>>>>> URL: http://svn.apache.org/viewvc/ofbiz/ofbiz-framework/trunk/
>>>>>> applications/datamodel/data/seed/ProductSeedData.xml?rev=
>>>>>> 1838320&r1=1838319&r2=1838320&view=diff
>>>>>> ============================================================
>>>>>> ==================
>>>>>> --- ofbiz/ofbiz-framework/trunk/applications/datamodel/data/
>>>>> seed/ProductSeedData.xml
>>>>>> (original)
>>>>>> +++ ofbiz/ofbiz-framework/trunk/applications/datamodel/data/
>>>>> seed/ProductSeedData.xml
>>>>>> Sat Aug 18 08:34:18 2018
>>>>>> @@ -281,6 +281,7 @@ under the License.
>>>>>>      <ProductPriceType description="Minimum Order Price"
>>>>>> productPriceTypeId="MINIMUM_ORDER_PRICE"/>
>>>>>>      <ProductPriceType description="Shipping Allowance Price"
>>>>>> productPriceTypeId="SHIPPING_ALLOWANCE"/>
>>>>>>
>>>>>> +    <ProductPricePurpose description="Deposit price"
>>>>>> productPricePurposeId="DEPOSIT"/>
>>>>>>      <ProductPricePurpose description="Purchase/Initial"
>>>>>> productPricePurposeId="PURCHASE"/>
>>>>>>      <ProductPricePurpose description="Recurring Charge"
>>>>>> productPricePurposeId="RECURRING_CHARGE"/>
>>>>>>      <ProductPricePurpose description="Usage Charge"
>>>>>> productPricePurposeId="USAGE_CHARGE"/>
>>>>>>
>>>>>> Modified: ofbiz/ofbiz-framework/trunk/applications/order/src/main/
>>>>>> java/org/apache/ofbiz/order/shoppingcart/ShoppingCartItem.java
>>>>>> URL: http://svn.apache.org/viewvc/ofbiz/ofbiz-framework/trunk/
>>>>>> applications/order/src/main/java/org/apache/ofbiz/order/
>>>>>> shoppingcart/ShoppingCartItem.java?rev=1838320&r1=1838319&
>>>>>> r2=1838320&view=diff
>>>>>> ============================================================
>>>>>> ==================
>>>>>> --- ofbiz/ofbiz-framework/trunk/applications/order/src/main/
>>>>>> java/org/apache/ofbiz/order/shoppingcart/ShoppingCartItem.java
>>>>> (original)
>>>>>> +++ ofbiz/ofbiz-framework/trunk/applications/order/src/main/
>>>>>> java/org/apache/ofbiz/order/shoppingcart/ShoppingCartItem.java Sat
>> Aug
>>>>> 18
>>>>>> 08:34:18 2018
>>>>>> @@ -1038,6 +1038,7 @@ public class ShoppingCartItem implements
>>>>>>              ProductPromoWorker.doPromotions(cart, dispatcher);
>>>>>>          }
>>>>>>
>>>>>> +        calcDepositAdjustments();
>>>>>>          if (!"PURCHASE_ORDER".equals(cart.getOrderType())) {
>>>>>>              // store the auto-save cart
>>>>>>              if (triggerExternalOps && ProductStoreWorker.
>>>>> autoSaveCart(delegator,
>>>>>> productStoreId)) {
>>>>>> @@ -1061,6 +1062,33 @@ public class ShoppingCartItem implements
>>>>>>          }
>>>>>>      }
>>>>>>
>>>>>> +    public void calcDepositAdjustments() {
>>>>>> +        List<GenericValue>itemAdjustments = this.getAdjustments();
>>>>>> +        try {
>>>>>> +            GenericValue depositAmount = EntityQuery.use(delegator).
>>>>>> from("ProductPrice").where("productId", this.getProductId(),
>>>>>> "productPricePurposeId", "DEPOSIT", "productPriceTypeId",
>>>>>> "DEFAULT_PRICE").filterByDate().queryFirst();
>>>>>> +            if (UtilValidate.isNotEmpty(depositAmount)) {
>>>>>> +                Boolean updatedDepositAmount = false;
>>>>>> +                BigDecimal adjustmentAmount =
>>>>>> depositAmount.getBigDecimal("price").multiply(this.getQuantity(),
>>>>>> generalRounding);
>>>>>> +                // itemAdjustments is a reference so directly setting
>>>>>> updated amount to the same.
>>>>>> +                    for(GenericValue itemAdjustment :
>>>> itemAdjustments) {
>>>>>> +                    if("DEPOSIT_ADJUSTMENT".equals(itemAdjustment.
>>>>>> getString("orderAdjustmentTypeId"))) {
>>>>>> +                            itemAdjustment.set("amount",
>>>>>> adjustmentAmount);
>>>>>> +                            updatedDepositAmount = true;
>>>>>> +                        }
>>>>>> +                }
>>>>>> +                if (!updatedDepositAmount) {
>>>>>> +                    GenericValue orderAdjustment =
>>>> delegator.makeValue("
>>>>>> OrderAdjustment");
>>>>>> +                    orderAdjustment.set("orderAdjustmentTypeId",
>>>>>> "DEPOSIT_ADJUSTMENT");
>>>>>> +                    orderAdjustment.set("description", "Surcharge
>>>>>> Adjustment");
>>>>>> +                    orderAdjustment.set("amount", adjustmentAmount);
>>>>>> +                    this.addAdjustment(orderAdjustment);
>>>>>> +                }
>>>>>> +            }
>>>>>> +        } catch (GenericEntityException e){
>>>>>> +            Debug.logError("Error in fetching deposite price
>>>> details!!",
>>>>>> module);
>>>>>> +        }
>>>>>> +    }
>>>>>> +
>>>>>>      public void updatePrice(LocalDispatcher dispatcher, ShoppingCart
>>>>>> cart) throws CartItemModifyException {
>>>>>>          // set basePrice using the calculateProductPrice service
>>>>>>          if (_product != null && isModifiedPrice == false) {
>>>>>>
>>>>>>
>>>>>>


smime.p7s (5K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: svn commit: r1838320 - in /ofbiz/ofbiz-framework/trunk/applications: datamodel/data/seed/OrderSeedData.xml datamodel/data/seed/ProductSeedData.xml order/src/main/java/org/apache/ofbiz/order/shoppingcart/ShoppingCartItem.java

Nicolas Malin-2
I assist Michael's response

On 19/08/2018 11:28, Michael Brohl wrote:
>
> If you have additional requirements, you are encouraged to provide
> solutions or concepts for them.
Be positive, be happy
I added just, if you not understand an improvement because comment or
example are too poor, help the commiter to understand the problem with
reformulate by example your vision, your last comment [1] sure is write
understandably for you, but not for me as regard the issue.

[1]
https://issues.apache.org/jira/browse/OFBIZ-7482?focusedCommentId=16584721&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-16584721

Nicolas
Reply | Threaded
Open this post in threaded view
|

OFBiz Contributors Best Practices was [Re: svn commit: r1838320 ...]

Jacques Le Roux
Administrator
In reply to this post by Michael Brohl-3
Hi All,

Yesterday Suraj asked me on HipChat what to do about OFBIZ-7598 <https://issues.apache.org/jira/browse/OFBIZ-7598> where Gil spotted an issue. Here is
my answer:

    Hi @SurajKhurana , we have not a clearly established policy for that case. Should we reuse the current Jira or should we rather create a new Jira
    to fix the issue? It seems to me though that we should rather create a new Jira because, after the commit, it's a fix not an improvement . This
    Jira should have a "Broken By" link referring to the original Jira. I see that Gil reopened the original Jira so better to close it also. It makes
    things more clear.
    While at it, I'll create a thread about this point in dev ML to get a consensus before writing this policy in
    https://cwiki.apache.org/confluence/display/OFBIZ/OFBiz+Contributors+Best+Practices
    <https://cwiki.apache.org/confluence/display/OFBIZ/OFBiz+Contributors+Best+Practices>

I shamelessly hijack this thread because it seems related to me and Michael have good points below (he also was the 1st to clearly suggest the
solution I advised to Suraj in HipChat).
And I believe that, to avoid rehearsing things, we should rewrite, as concisely as possible, what Michael suggests below (not an easy thing)

Thoughts?

And I agree with Nicolas, helping others is always good. At some point you will maybe even find helping yourself in the future.
Damn sometimes I can't even understand my own code... never happened to you?... This may help: https://twitter.com/romiem/status/1030438339390910464 
https://twitter.com/romiem/status/1030438
339390910464 (I sure have the last book on end)

Enjoy the rest of your weekend :)

Jacques

Le 19/08/2018 à 11:28, Michael Brohl a écrit :

> I have not the time to dig into the specific details right now so will just give my thoughts on the process in general because of the citations:
>
> 1. we have to distinguish between (a) completely new functionality or major refactorings and (b) the enhancement of functionality which is already
> in the code base
>
> 2. for (a), we should first have consenus that we want the proposed solution and we should look for a complete, well designed and carefully tested
> solution before the first commit will be done. This is to prevent the introduction of new problematic code.
>
> 3. for (b), I think every improvement of existing code/functionality helps and should be committed if there are no flaws in the design or technical
> solution and it does not break existing funtionality. of course, it should be complete within the *scope* of the improvement.
>
> 4. if the solution for (b) does not cover other wishes or things which could be enhanced also, this would be no reason to not commit the
> improvement. If people have further requirements, they can provide concepts and solutions/patches anytime to make things better.
>
> In this case, for me it is important if Suraj's commit
>
> a. breaks anything?
>
> b. is vetoed by other committers in view of code quality or design flaws?
>
> If none of these questions get a "yes", then I see no reason to revert.
>
> If you have additional requirements, you are encouraged to provide solutions or concepts for them.
>
> Thanks,
>
> Michael Brohl
> ecomify GmbH
> www.ecomify.de
>
>
> Am 19.08.18 um 09:25 schrieb Pierre Smits:
>> Please read the comment in the related ticket [1]
>>
>> https://issues.apache.org/jira/browse/OFBIZ-7482?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=16584721#comment-16584721 
>>
>>
>>
>> Best regards,
>>
>> Pierre Smits
>>
>> Apache Trafodion <https://trafodion.apache.org>, Vice President
>> Apache Directory <https://directory.apache.org>, PMC Member
>> Apache Incubator <https://incubator.apache.org>, committer
>> *Apache OFBiz <https://ofbiz.apache.org>, contributor (without privileges)
>> since 2008*
>> Apache Steve <https://steve.apache.org>, committer
>>
>> On Sat, Aug 18, 2018 at 9:01 PM, Michael Brohl <[hidden email]>
>> wrote:
>>
>>> How do these citations relate to Suraj‘s work?
>>>
>>> Please provide arguments in your own words if you think someone‘s work has
>>> flaws and should be reverted.
>>>
>>> Thanks,
>>> Michael
>>>
>>>
>>>> Am 18.08.2018 um 13:54 schrieb Pierre Smits <[hidden email]>:
>>>>
>>>> As Michael recently pointed out in another thread:
>>>>
>>>> {quote}
>>>>
>>>> *If it does break anything or introduces functionality which is not
>>> working
>>>> completely, we should revert.*
>>>>
>>>> {quote}
>>>>
>>>> And:
>>>>
>>>> {quote}
>>>>
>>>> *We are struggling with half baked, incomplete or buggy code in several
>>>> areas which often shows up a long time after it was committed. In other
>>>> cases, even if problems are raised soon after the commit, the code is
>>> left
>>>> untouched for a long time until it is work on again or simply is
>>> forgotten.*
>>>> {quote}
>>>>
>>>>
>>>> Best regards,
>>>>
>>>> Pierre Smits
>>>>
>>>> On Sat, Aug 18, 2018 at 10:52 AM, Suraj Khurana <
>>>> [hidden email]> wrote:
>>>>
>>>>> Hi Pierre,
>>>>>
>>>>> This is not a new patch, this is updated version of two years old patch
>>>>> which has been already reviewed if you follow comments on ticket.
>>>>> We need to add updated patch as well since many file path have been
>>> changed
>>>>> and we have data files refactoring as well.
>>>>>
>>>>> HTH.
>>>>> --
>>>>> Best Regards,
>>>>> Suraj Khurana | Omni-channel OMS Technical Expert
>>>>> HotWax Commerce  by  HotWax Systems
>>>>>
>>>>> On Sat, Aug 18, 2018 at 2:13 PM, Pierre Smits <[hidden email]>
>>>>> wrote:
>>>>>
>>>>>> Hi Suraj,
>>>>>>
>>>>>> Please revert! Within 10 minutes you posted a new patch and committed
>>> it
>>>>> to
>>>>>> trunk and closed the issue. It is customary to follow the 72 hr delay
>>>>> rule
>>>>>> to allow the community to review the changes and assess the impact.
>>>>>>
>>>>>>
>>>>>> Best regards,
>>>>>>
>>>>>> Pierre Smits
>>>>>>
>>>>>> Apache Trafodion <https://trafodion.apache.org>, Vice President
>>>>>> Apache Directory <https://directory.apache.org>, PMC Member
>>>>>> Apache Incubator <https://incubator.apache.org>, committer
>>>>>> Apache OFBiz <https://ofbiz.apache.org>, contributor since 2008
>>>>>> Apache Steve <https://steve.apache.org>, committer
>>>>>>
>>>>>>> On Sat, Aug 18, 2018 at 10:34 AM, <[hidden email]> wrote:
>>>>>>>
>>>>>>> Author: surajk
>>>>>>> Date: Sat Aug 18 08:34:18 2018
>>>>>>> New Revision: 1838320
>>>>>>>
>>>>>>> URL: http://svn.apache.org/viewvc?rev=1838320&view=rev
>>>>>>> Log:
>>>>>>> Improved: Added support to calculate deposit price as well while
>>>>> creating
>>>>>>> shopping cart item.
>>>>>>> (OFBIZ-7482)
>>>>>>>
>>>>>>> Modified:
>>>>>>> ofbiz/ofbiz-framework/trunk/applications/datamodel/data/
>>>>>>> seed/OrderSeedData.xml
>>>>>>> ofbiz/ofbiz-framework/trunk/applications/datamodel/data/
>>>>>>> seed/ProductSeedData.xml
>>>>>>> ofbiz/ofbiz-framework/trunk/applications/order/src/main/
>>>>>>> java/org/apache/ofbiz/order/shoppingcart/ShoppingCartItem.java
>>>>>>>
>>>>>>> Modified: ofbiz/ofbiz-framework/trunk/applications/datamodel/data/
>>>>>>> seed/OrderSeedData.xml
>>>>>>> URL: http://svn.apache.org/viewvc/ofbiz/ofbiz-framework/trunk/
>>>>>>> applications/datamodel/data/seed/OrderSeedData.xml?rev=
>>>>>>> 1838320&r1=1838319&r2=1838320&view=diff
>>>>>>> ============================================================
>>>>>>> ==================
>>>>>>> --- ofbiz/ofbiz-framework/trunk/applications/datamodel/data/
>>>>>> seed/OrderSeedData.xml
>>>>>>> (original)
>>>>>>> +++ ofbiz/ofbiz-framework/trunk/applications/datamodel/data/
>>>>>> seed/OrderSeedData.xml
>>>>>>> Sat Aug 18 08:34:18 2018
>>>>>>> @@ -49,6 +49,7 @@ under the License.
>>>>>>>      <OrderAdjustmentType description="Additional Feature" hasTable="N"
>>>>>>> orderAdjustmentTypeId="ADDITIONAL_FEATURE"/>
>>>>>>>      <OrderAdjustmentType description="Warranty" hasTable="N"
>>>>>>> orderAdjustmentTypeId="WARRANTY_ADJUSTMENT"/>
>>>>>>>      <OrderAdjustmentType description="Marketing Package Adjustment"
>>>>>>> hasTable="N" orderAdjustmentTypeId="MKTG_PKG_AUTO_ADJUST"/>
>>>>>>> +    <OrderAdjustmentType description="Deposit" hasTable="N"
>>>>>>> orderAdjustmentTypeId="DEPOSIT_ADJUSTMENT"/>
>>>>>>>
>>>>>>>      <OrderBlacklistType orderBlacklistTypeId="BLACKLIST_ADDRESS"
>>>>>>> description="Addresss"/>
>>>>>>>      <OrderBlacklistType orderBlacklistTypeId="BLACKLIST_CREDITCARD"
>>>>>>> description="Credit Card"/>
>>>>>>>
>>>>>>> Modified: ofbiz/ofbiz-framework/trunk/applications/datamodel/data/
>>>>>>> seed/ProductSeedData.xml
>>>>>>> URL: http://svn.apache.org/viewvc/ofbiz/ofbiz-framework/trunk/
>>>>>>> applications/datamodel/data/seed/ProductSeedData.xml?rev=
>>>>>>> 1838320&r1=1838319&r2=1838320&view=diff
>>>>>>> ============================================================
>>>>>>> ==================
>>>>>>> --- ofbiz/ofbiz-framework/trunk/applications/datamodel/data/
>>>>>> seed/ProductSeedData.xml
>>>>>>> (original)
>>>>>>> +++ ofbiz/ofbiz-framework/trunk/applications/datamodel/data/
>>>>>> seed/ProductSeedData.xml
>>>>>>> Sat Aug 18 08:34:18 2018
>>>>>>> @@ -281,6 +281,7 @@ under the License.
>>>>>>>      <ProductPriceType description="Minimum Order Price"
>>>>>>> productPriceTypeId="MINIMUM_ORDER_PRICE"/>
>>>>>>>      <ProductPriceType description="Shipping Allowance Price"
>>>>>>> productPriceTypeId="SHIPPING_ALLOWANCE"/>
>>>>>>>
>>>>>>> +    <ProductPricePurpose description="Deposit price"
>>>>>>> productPricePurposeId="DEPOSIT"/>
>>>>>>>      <ProductPricePurpose description="Purchase/Initial"
>>>>>>> productPricePurposeId="PURCHASE"/>
>>>>>>>      <ProductPricePurpose description="Recurring Charge"
>>>>>>> productPricePurposeId="RECURRING_CHARGE"/>
>>>>>>>      <ProductPricePurpose description="Usage Charge"
>>>>>>> productPricePurposeId="USAGE_CHARGE"/>
>>>>>>>
>>>>>>> Modified: ofbiz/ofbiz-framework/trunk/applications/order/src/main/
>>>>>>> java/org/apache/ofbiz/order/shoppingcart/ShoppingCartItem.java
>>>>>>> URL: http://svn.apache.org/viewvc/ofbiz/ofbiz-framework/trunk/
>>>>>>> applications/order/src/main/java/org/apache/ofbiz/order/
>>>>>>> shoppingcart/ShoppingCartItem.java?rev=1838320&r1=1838319&
>>>>>>> r2=1838320&view=diff
>>>>>>> ============================================================
>>>>>>> ==================
>>>>>>> --- ofbiz/ofbiz-framework/trunk/applications/order/src/main/
>>>>>>> java/org/apache/ofbiz/order/shoppingcart/ShoppingCartItem.java
>>>>>> (original)
>>>>>>> +++ ofbiz/ofbiz-framework/trunk/applications/order/src/main/
>>>>>>> java/org/apache/ofbiz/order/shoppingcart/ShoppingCartItem.java Sat
>>> Aug
>>>>>> 18
>>>>>>> 08:34:18 2018
>>>>>>> @@ -1038,6 +1038,7 @@ public class ShoppingCartItem implements
>>>>>>>              ProductPromoWorker.doPromotions(cart, dispatcher);
>>>>>>>          }
>>>>>>>
>>>>>>> +        calcDepositAdjustments();
>>>>>>>          if (!"PURCHASE_ORDER".equals(cart.getOrderType())) {
>>>>>>>              // store the auto-save cart
>>>>>>>              if (triggerExternalOps && ProductStoreWorker.
>>>>>> autoSaveCart(delegator,
>>>>>>> productStoreId)) {
>>>>>>> @@ -1061,6 +1062,33 @@ public class ShoppingCartItem implements
>>>>>>>          }
>>>>>>>      }
>>>>>>>
>>>>>>> +    public void calcDepositAdjustments() {
>>>>>>> +        List<GenericValue>itemAdjustments = this.getAdjustments();
>>>>>>> +        try {
>>>>>>> +            GenericValue depositAmount = EntityQuery.use(delegator).
>>>>>>> from("ProductPrice").where("productId", this.getProductId(),
>>>>>>> "productPricePurposeId", "DEPOSIT", "productPriceTypeId",
>>>>>>> "DEFAULT_PRICE").filterByDate().queryFirst();
>>>>>>> +            if (UtilValidate.isNotEmpty(depositAmount)) {
>>>>>>> +                Boolean updatedDepositAmount = false;
>>>>>>> +                BigDecimal adjustmentAmount =
>>>>>>> depositAmount.getBigDecimal("price").multiply(this.getQuantity(),
>>>>>>> generalRounding);
>>>>>>> +                // itemAdjustments is a reference so directly setting
>>>>>>> updated amount to the same.
>>>>>>> +                    for(GenericValue itemAdjustment :
>>>>> itemAdjustments) {
>>>>>>> + if("DEPOSIT_ADJUSTMENT".equals(itemAdjustment.
>>>>>>> getString("orderAdjustmentTypeId"))) {
>>>>>>> + itemAdjustment.set("amount",
>>>>>>> adjustmentAmount);
>>>>>>> +                            updatedDepositAmount = true;
>>>>>>> +                        }
>>>>>>> +                }
>>>>>>> +                if (!updatedDepositAmount) {
>>>>>>> +                    GenericValue orderAdjustment =
>>>>> delegator.makeValue("
>>>>>>> OrderAdjustment");
>>>>>>> + orderAdjustment.set("orderAdjustmentTypeId",
>>>>>>> "DEPOSIT_ADJUSTMENT");
>>>>>>> + orderAdjustment.set("description", "Surcharge
>>>>>>> Adjustment");
>>>>>>> +                    orderAdjustment.set("amount", adjustmentAmount);
>>>>>>> + this.addAdjustment(orderAdjustment);
>>>>>>> +                }
>>>>>>> +            }
>>>>>>> +        } catch (GenericEntityException e){
>>>>>>> +            Debug.logError("Error in fetching deposite price
>>>>> details!!",
>>>>>>> module);
>>>>>>> +        }
>>>>>>> +    }
>>>>>>> +
>>>>>>>      public void updatePrice(LocalDispatcher dispatcher, ShoppingCart
>>>>>>> cart) throws CartItemModifyException {
>>>>>>>          // set basePrice using the calculateProductPrice service
>>>>>>>          if (_product != null && isModifiedPrice == false) {
>>>>>>>
>>>>>>>
>>>>>>>
>
>

Reply | Threaded
Open this post in threaded view
|

Re: OFBiz Contributors Best Practices was [Re: svn commit: r1838320 ...]

Gil Portenseigne
Hello Jacques, All,

I do not agree about creating a new jira, since the improvement has
design flaw (i will add a comment into the Jira introducing it) in my
opinion leading to breaking thread safety in ModelForm.java.
And Taking into consideration Michael preference for reverting,
the best way to go in my opinion, is to revert and rework the feature.
I'd be glad to help out with that :).

All details here : https://s.apache.org/C9VF (within the quotation for
thread safe matter)

For clarifying the policy and writing it down +1

Gil

Le dimanche 19 août 2018 à 14:49:31 (+0200), Jacques Le Roux a écrit :

> Hi All,
>
> Yesterday Suraj asked me on HipChat what to do about OFBIZ-7598
> <https://issues.apache.org/jira/browse/OFBIZ-7598> where Gil spotted an
> issue. Here is my answer:
>
>    Hi @SurajKhurana , we have not a clearly established policy for that case. Should we reuse the current Jira or should we rather create a new Jira
>    to fix the issue? It seems to me though that we should rather create a new Jira because, after the commit, it's a fix not an improvement . This
>    Jira should have a "Broken By" link referring to the original Jira. I see that Gil reopened the original Jira so better to close it also. It makes
>    things more clear.
>    While at it, I'll create a thread about this point in dev ML to get a consensus before writing this policy in
>    https://cwiki.apache.org/confluence/display/OFBIZ/OFBiz+Contributors+Best+Practices
>    <https://cwiki.apache.org/confluence/display/OFBIZ/OFBiz+Contributors+Best+Practices>
>
> I shamelessly hijack this thread because it seems related to me and Michael
> have good points below (he also was the 1st to clearly suggest the solution
> I advised to Suraj in HipChat).
> And I believe that, to avoid rehearsing things, we should rewrite, as concisely as possible, what Michael suggests below (not an easy thing)
>
> Thoughts?
>
> And I agree with Nicolas, helping others is always good. At some point you will maybe even find helping yourself in the future.
> Damn sometimes I can't even understand my own code... never happened to
> you?... This may help: https://twitter.com/romiem/status/1030438339390910464
> https://twitter.com/romiem/status/1030438
> 339390910464 (I sure have the last book on end)
>
> Enjoy the rest of your weekend :)
>
> Jacques
>
> Le 19/08/2018 à 11:28, Michael Brohl a écrit :
> > I have not the time to dig into the specific details right now so will just give my thoughts on the process in general because of the citations:
> >
> > 1. we have to distinguish between (a) completely new functionality or
> > major refactorings and (b) the enhancement of functionality which is
> > already in the code base
> >
> > 2. for (a), we should first have consenus that we want the proposed
> > solution and we should look for a complete, well designed and carefully
> > tested solution before the first commit will be done. This is to prevent
> > the introduction of new problematic code.
> >
> > 3. for (b), I think every improvement of existing code/functionality
> > helps and should be committed if there are no flaws in the design or
> > technical solution and it does not break existing funtionality. of
> > course, it should be complete within the *scope* of the improvement.
> >
> > 4. if the solution for (b) does not cover other wishes or things which
> > could be enhanced also, this would be no reason to not commit the
> > improvement. If people have further requirements, they can provide
> > concepts and solutions/patches anytime to make things better.
> >
> > In this case, for me it is important if Suraj's commit
> >
> > a. breaks anything?
> >
> > b. is vetoed by other committers in view of code quality or design flaws?
> >
> > If none of these questions get a "yes", then I see no reason to revert.
> >
> > If you have additional requirements, you are encouraged to provide solutions or concepts for them.
> >
> > Thanks,
> >
> > Michael Brohl
> > ecomify GmbH
> > www.ecomify.de
> >
> >
> > Am 19.08.18 um 09:25 schrieb Pierre Smits:
> > > Please read the comment in the related ticket [1]
> > >
> > > https://issues.apache.org/jira/browse/OFBIZ-7482?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=16584721#comment-16584721
> > >
> > >
> > >
> > > Best regards,
> > >
> > > Pierre Smits
> > >
> > > Apache Trafodion <https://trafodion.apache.org>, Vice President
> > > Apache Directory <https://directory.apache.org>, PMC Member
> > > Apache Incubator <https://incubator.apache.org>, committer
> > > *Apache OFBiz <https://ofbiz.apache.org>, contributor (without privileges)
> > > since 2008*
> > > Apache Steve <https://steve.apache.org>, committer
> > >
> > > On Sat, Aug 18, 2018 at 9:01 PM, Michael Brohl <[hidden email]>
> > > wrote:
> > >
> > > > How do these citations relate to Suraj‘s work?
> > > >
> > > > Please provide arguments in your own words if you think someone‘s work has
> > > > flaws and should be reverted.
> > > >
> > > > Thanks,
> > > > Michael
> > > >
> > > >
> > > > > Am 18.08.2018 um 13:54 schrieb Pierre Smits <[hidden email]>:
> > > > >
> > > > > As Michael recently pointed out in another thread:
> > > > >
> > > > > {quote}
> > > > >
> > > > > *If it does break anything or introduces functionality which is not
> > > > working
> > > > > completely, we should revert.*
> > > > >
> > > > > {quote}
> > > > >
> > > > > And:
> > > > >
> > > > > {quote}
> > > > >
> > > > > *We are struggling with half baked, incomplete or buggy code in several
> > > > > areas which often shows up a long time after it was committed. In other
> > > > > cases, even if problems are raised soon after the commit, the code is
> > > > left
> > > > > untouched for a long time until it is work on again or simply is
> > > > forgotten.*
> > > > > {quote}
> > > > >
> > > > >
> > > > > Best regards,
> > > > >
> > > > > Pierre Smits
> > > > >
> > > > > On Sat, Aug 18, 2018 at 10:52 AM, Suraj Khurana <
> > > > > [hidden email]> wrote:
> > > > >
> > > > > > Hi Pierre,
> > > > > >
> > > > > > This is not a new patch, this is updated version of two years old patch
> > > > > > which has been already reviewed if you follow comments on ticket.
> > > > > > We need to add updated patch as well since many file path have been
> > > > changed
> > > > > > and we have data files refactoring as well.
> > > > > >
> > > > > > HTH.
> > > > > > --
> > > > > > Best Regards,
> > > > > > Suraj Khurana | Omni-channel OMS Technical Expert
> > > > > > HotWax Commerce  by  HotWax Systems
> > > > > >
> > > > > > On Sat, Aug 18, 2018 at 2:13 PM, Pierre Smits <[hidden email]>
> > > > > > wrote:
> > > > > >
> > > > > > > Hi Suraj,
> > > > > > >
> > > > > > > Please revert! Within 10 minutes you posted a new patch and committed
> > > > it
> > > > > > to
> > > > > > > trunk and closed the issue. It is customary to follow the 72 hr delay
> > > > > > rule
> > > > > > > to allow the community to review the changes and assess the impact.
> > > > > > >
> > > > > > >
> > > > > > > Best regards,
> > > > > > >
> > > > > > > Pierre Smits
> > > > > > >
> > > > > > > Apache Trafodion <https://trafodion.apache.org>, Vice President
> > > > > > > Apache Directory <https://directory.apache.org>, PMC Member
> > > > > > > Apache Incubator <https://incubator.apache.org>, committer
> > > > > > > Apache OFBiz <https://ofbiz.apache.org>, contributor since 2008
> > > > > > > Apache Steve <https://steve.apache.org>, committer
> > > > > > >
> > > > > > > > On Sat, Aug 18, 2018 at 10:34 AM, <[hidden email]> wrote:
> > > > > > > >
> > > > > > > > Author: surajk
> > > > > > > > Date: Sat Aug 18 08:34:18 2018
> > > > > > > > New Revision: 1838320
> > > > > > > >
> > > > > > > > URL: http://svn.apache.org/viewvc?rev=1838320&view=rev
> > > > > > > > Log:
> > > > > > > > Improved: Added support to calculate deposit price as well while
> > > > > > creating
> > > > > > > > shopping cart item.
> > > > > > > > (OFBIZ-7482)
> > > > > > > >
> > > > > > > > Modified:
> > > > > > > > ofbiz/ofbiz-framework/trunk/applications/datamodel/data/
> > > > > > > > seed/OrderSeedData.xml
> > > > > > > > ofbiz/ofbiz-framework/trunk/applications/datamodel/data/
> > > > > > > > seed/ProductSeedData.xml
> > > > > > > > ofbiz/ofbiz-framework/trunk/applications/order/src/main/
> > > > > > > > java/org/apache/ofbiz/order/shoppingcart/ShoppingCartItem.java
> > > > > > > >
> > > > > > > > Modified: ofbiz/ofbiz-framework/trunk/applications/datamodel/data/
> > > > > > > > seed/OrderSeedData.xml
> > > > > > > > URL: http://svn.apache.org/viewvc/ofbiz/ofbiz-framework/trunk/
> > > > > > > > applications/datamodel/data/seed/OrderSeedData.xml?rev=
> > > > > > > > 1838320&r1=1838319&r2=1838320&view=diff
> > > > > > > > ============================================================
> > > > > > > > ==================
> > > > > > > > --- ofbiz/ofbiz-framework/trunk/applications/datamodel/data/
> > > > > > > seed/OrderSeedData.xml
> > > > > > > > (original)
> > > > > > > > +++ ofbiz/ofbiz-framework/trunk/applications/datamodel/data/
> > > > > > > seed/OrderSeedData.xml
> > > > > > > > Sat Aug 18 08:34:18 2018
> > > > > > > > @@ -49,6 +49,7 @@ under the License.
> > > > > > > >      <OrderAdjustmentType description="Additional Feature" hasTable="N"
> > > > > > > > orderAdjustmentTypeId="ADDITIONAL_FEATURE"/>
> > > > > > > >      <OrderAdjustmentType description="Warranty" hasTable="N"
> > > > > > > > orderAdjustmentTypeId="WARRANTY_ADJUSTMENT"/>
> > > > > > > >      <OrderAdjustmentType description="Marketing Package Adjustment"
> > > > > > > > hasTable="N" orderAdjustmentTypeId="MKTG_PKG_AUTO_ADJUST"/>
> > > > > > > > +    <OrderAdjustmentType description="Deposit" hasTable="N"
> > > > > > > > orderAdjustmentTypeId="DEPOSIT_ADJUSTMENT"/>
> > > > > > > >
> > > > > > > >      <OrderBlacklistType orderBlacklistTypeId="BLACKLIST_ADDRESS"
> > > > > > > > description="Addresss"/>
> > > > > > > >      <OrderBlacklistType orderBlacklistTypeId="BLACKLIST_CREDITCARD"
> > > > > > > > description="Credit Card"/>
> > > > > > > >
> > > > > > > > Modified: ofbiz/ofbiz-framework/trunk/applications/datamodel/data/
> > > > > > > > seed/ProductSeedData.xml
> > > > > > > > URL: http://svn.apache.org/viewvc/ofbiz/ofbiz-framework/trunk/
> > > > > > > > applications/datamodel/data/seed/ProductSeedData.xml?rev=
> > > > > > > > 1838320&r1=1838319&r2=1838320&view=diff
> > > > > > > > ============================================================
> > > > > > > > ==================
> > > > > > > > --- ofbiz/ofbiz-framework/trunk/applications/datamodel/data/
> > > > > > > seed/ProductSeedData.xml
> > > > > > > > (original)
> > > > > > > > +++ ofbiz/ofbiz-framework/trunk/applications/datamodel/data/
> > > > > > > seed/ProductSeedData.xml
> > > > > > > > Sat Aug 18 08:34:18 2018
> > > > > > > > @@ -281,6 +281,7 @@ under the License.
> > > > > > > >      <ProductPriceType description="Minimum Order Price"
> > > > > > > > productPriceTypeId="MINIMUM_ORDER_PRICE"/>
> > > > > > > >      <ProductPriceType description="Shipping Allowance Price"
> > > > > > > > productPriceTypeId="SHIPPING_ALLOWANCE"/>
> > > > > > > >
> > > > > > > > +    <ProductPricePurpose description="Deposit price"
> > > > > > > > productPricePurposeId="DEPOSIT"/>
> > > > > > > >      <ProductPricePurpose description="Purchase/Initial"
> > > > > > > > productPricePurposeId="PURCHASE"/>
> > > > > > > >      <ProductPricePurpose description="Recurring Charge"
> > > > > > > > productPricePurposeId="RECURRING_CHARGE"/>
> > > > > > > >      <ProductPricePurpose description="Usage Charge"
> > > > > > > > productPricePurposeId="USAGE_CHARGE"/>
> > > > > > > >
> > > > > > > > Modified: ofbiz/ofbiz-framework/trunk/applications/order/src/main/
> > > > > > > > java/org/apache/ofbiz/order/shoppingcart/ShoppingCartItem.java
> > > > > > > > URL: http://svn.apache.org/viewvc/ofbiz/ofbiz-framework/trunk/
> > > > > > > > applications/order/src/main/java/org/apache/ofbiz/order/
> > > > > > > > shoppingcart/ShoppingCartItem.java?rev=1838320&r1=1838319&
> > > > > > > > r2=1838320&view=diff
> > > > > > > > ============================================================
> > > > > > > > ==================
> > > > > > > > --- ofbiz/ofbiz-framework/trunk/applications/order/src/main/
> > > > > > > > java/org/apache/ofbiz/order/shoppingcart/ShoppingCartItem.java
> > > > > > > (original)
> > > > > > > > +++ ofbiz/ofbiz-framework/trunk/applications/order/src/main/
> > > > > > > > java/org/apache/ofbiz/order/shoppingcart/ShoppingCartItem.java Sat
> > > > Aug
> > > > > > > 18
> > > > > > > > 08:34:18 2018
> > > > > > > > @@ -1038,6 +1038,7 @@ public class ShoppingCartItem implements
> > > > > > > >              ProductPromoWorker.doPromotions(cart, dispatcher);
> > > > > > > >          }
> > > > > > > >
> > > > > > > > +        calcDepositAdjustments();
> > > > > > > >          if (!"PURCHASE_ORDER".equals(cart.getOrderType())) {
> > > > > > > >              // store the auto-save cart
> > > > > > > >              if (triggerExternalOps && ProductStoreWorker.
> > > > > > > autoSaveCart(delegator,
> > > > > > > > productStoreId)) {
> > > > > > > > @@ -1061,6 +1062,33 @@ public class ShoppingCartItem implements
> > > > > > > >          }
> > > > > > > >      }
> > > > > > > >
> > > > > > > > +    public void calcDepositAdjustments() {
> > > > > > > > +        List<GenericValue>itemAdjustments = this.getAdjustments();
> > > > > > > > +        try {
> > > > > > > > +            GenericValue depositAmount = EntityQuery.use(delegator).
> > > > > > > > from("ProductPrice").where("productId", this.getProductId(),
> > > > > > > > "productPricePurposeId", "DEPOSIT", "productPriceTypeId",
> > > > > > > > "DEFAULT_PRICE").filterByDate().queryFirst();
> > > > > > > > +            if (UtilValidate.isNotEmpty(depositAmount)) {
> > > > > > > > +                Boolean updatedDepositAmount = false;
> > > > > > > > +                BigDecimal adjustmentAmount =
> > > > > > > > depositAmount.getBigDecimal("price").multiply(this.getQuantity(),
> > > > > > > > generalRounding);
> > > > > > > > +                // itemAdjustments is a reference so directly setting
> > > > > > > > updated amount to the same.
> > > > > > > > +                    for(GenericValue itemAdjustment :
> > > > > > itemAdjustments) {
> > > > > > > > + if("DEPOSIT_ADJUSTMENT".equals(itemAdjustment.
> > > > > > > > getString("orderAdjustmentTypeId"))) {
> > > > > > > > + itemAdjustment.set("amount",
> > > > > > > > adjustmentAmount);
> > > > > > > > +                            updatedDepositAmount = true;
> > > > > > > > +                        }
> > > > > > > > +                }
> > > > > > > > +                if (!updatedDepositAmount) {
> > > > > > > > +                    GenericValue orderAdjustment =
> > > > > > delegator.makeValue("
> > > > > > > > OrderAdjustment");
> > > > > > > > + orderAdjustment.set("orderAdjustmentTypeId",
> > > > > > > > "DEPOSIT_ADJUSTMENT");
> > > > > > > > + orderAdjustment.set("description", "Surcharge
> > > > > > > > Adjustment");
> > > > > > > > +                    orderAdjustment.set("amount", adjustmentAmount);
> > > > > > > > + this.addAdjustment(orderAdjustment);
> > > > > > > > +                }
> > > > > > > > +            }
> > > > > > > > +        } catch (GenericEntityException e){
> > > > > > > > +            Debug.logError("Error in fetching deposite price
> > > > > > details!!",
> > > > > > > > module);
> > > > > > > > +        }
> > > > > > > > +    }
> > > > > > > > +
> > > > > > > >      public void updatePrice(LocalDispatcher dispatcher, ShoppingCart
> > > > > > > > cart) throws CartItemModifyException {
> > > > > > > >          // set basePrice using the calculateProductPrice service
> > > > > > > >          if (_product != null && isModifiedPrice == false) {
> > > > > > > >
> > > > > > > >
> > > > > > > >
> >
> >
>
Reply | Threaded
Open this post in threaded view
|

Re: OFBiz Contributors Best Practices was [Re: svn commit: r1838320 ...]

Jacques Le Roux
Administrator
Hi Gil,

Inline..

Le 20/08/2018 à 11:48, Gil Portenseigne a écrit :
> Hello Jacques, All,
>
> I do not agree about creating a new jira, since the improvement has
> design flaw (i will add a comment into the Jira introducing it) in my
> opinion leading to breaking thread safety in ModelForm.java.
That's indeed something we can't take lightly

> And Taking into consideration Michael preference for reverting,
> the best way to go in my opinion, is to revert and rework the feature.
> I'd be glad to help out with that :).
That sounds like the best solution in this case indeed.

> All details here : https://s.apache.org/C9VF (within the quotation for
> thread safe matter)
>
> For clarifying the policy and writing it down +1
OK thanks, l'll wait other opinions, and hopefully help, before rephrasing.

Jacques

>
> Gil
>
> Le dimanche 19 août 2018 à 14:49:31 (+0200), Jacques Le Roux a écrit :
>> Hi All,
>>
>> Yesterday Suraj asked me on HipChat what to do about OFBIZ-7598
>> <https://issues.apache.org/jira/browse/OFBIZ-7598> where Gil spotted an
>> issue. Here is my answer:
>>
>>     Hi @SurajKhurana , we have not a clearly established policy for that case. Should we reuse the current Jira or should we rather create a new Jira
>>     to fix the issue? It seems to me though that we should rather create a new Jira because, after the commit, it's a fix not an improvement . This
>>     Jira should have a "Broken By" link referring to the original Jira. I see that Gil reopened the original Jira so better to close it also. It makes
>>     things more clear.
>>     While at it, I'll create a thread about this point in dev ML to get a consensus before writing this policy in
>>     https://cwiki.apache.org/confluence/display/OFBIZ/OFBiz+Contributors+Best+Practices
>>     <https://cwiki.apache.org/confluence/display/OFBIZ/OFBiz+Contributors+Best+Practices>
>>
>> I shamelessly hijack this thread because it seems related to me and Michael
>> have good points below (he also was the 1st to clearly suggest the solution
>> I advised to Suraj in HipChat).
>> And I believe that, to avoid rehearsing things, we should rewrite, as concisely as possible, what Michael suggests below (not an easy thing)
>>
>> Thoughts?
>>
>> And I agree with Nicolas, helping others is always good. At some point you will maybe even find helping yourself in the future.
>> Damn sometimes I can't even understand my own code... never happened to
>> you?... This may help: https://twitter.com/romiem/status/1030438339390910464
>> https://twitter.com/romiem/status/1030438
>> 339390910464 (I sure have the last book on end)
>>
>> Enjoy the rest of your weekend :)
>>
>> Jacques
>>
>> Le 19/08/2018 à 11:28, Michael Brohl a écrit :
>>> I have not the time to dig into the specific details right now so will just give my thoughts on the process in general because of the citations:
>>>
>>> 1. we have to distinguish between (a) completely new functionality or
>>> major refactorings and (b) the enhancement of functionality which is
>>> already in the code base
>>>
>>> 2. for (a), we should first have consenus that we want the proposed
>>> solution and we should look for a complete, well designed and carefully
>>> tested solution before the first commit will be done. This is to prevent
>>> the introduction of new problematic code.
>>>
>>> 3. for (b), I think every improvement of existing code/functionality
>>> helps and should be committed if there are no flaws in the design or
>>> technical solution and it does not break existing funtionality. of
>>> course, it should be complete within the *scope* of the improvement.
>>>
>>> 4. if the solution for (b) does not cover other wishes or things which
>>> could be enhanced also, this would be no reason to not commit the
>>> improvement. If people have further requirements, they can provide
>>> concepts and solutions/patches anytime to make things better.
>>>
>>> In this case, for me it is important if Suraj's commit
>>>
>>> a. breaks anything?
>>>
>>> b. is vetoed by other committers in view of code quality or design flaws?
>>>
>>> If none of these questions get a "yes", then I see no reason to revert.
>>>
>>> If you have additional requirements, you are encouraged to provide solutions or concepts for them.
>>>
>>> Thanks,
>>>
>>> Michael Brohl
>>> ecomify GmbH
>>> www.ecomify.de
>>>
>>>
>>> Am 19.08.18 um 09:25 schrieb Pierre Smits:
>>>> Please read the comment in the related ticket [1]
>>>>
>>>> https://issues.apache.org/jira/browse/OFBIZ-7482?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=16584721#comment-16584721
>>>>
>>>>
>>>>
>>>> Best regards,
>>>>
>>>> Pierre Smits
>>>>
>>>> Apache Trafodion <https://trafodion.apache.org>, Vice President
>>>> Apache Directory <https://directory.apache.org>, PMC Member
>>>> Apache Incubator <https://incubator.apache.org>, committer
>>>> *Apache OFBiz <https://ofbiz.apache.org>, contributor (without privileges)
>>>> since 2008*
>>>> Apache Steve <https://steve.apache.org>, committer
>>>>
>>>> On Sat, Aug 18, 2018 at 9:01 PM, Michael Brohl <[hidden email]>
>>>> wrote:
>>>>
>>>>> How do these citations relate to Suraj‘s work?
>>>>>
>>>>> Please provide arguments in your own words if you think someone‘s work has
>>>>> flaws and should be reverted.
>>>>>
>>>>> Thanks,
>>>>> Michael
>>>>>
>>>>>
>>>>>> Am 18.08.2018 um 13:54 schrieb Pierre Smits <[hidden email]>:
>>>>>>
>>>>>> As Michael recently pointed out in another thread:
>>>>>>
>>>>>> {quote}
>>>>>>
>>>>>> *If it does break anything or introduces functionality which is not
>>>>> working
>>>>>> completely, we should revert.*
>>>>>>
>>>>>> {quote}
>>>>>>
>>>>>> And:
>>>>>>
>>>>>> {quote}
>>>>>>
>>>>>> *We are struggling with half baked, incomplete or buggy code in several
>>>>>> areas which often shows up a long time after it was committed. In other
>>>>>> cases, even if problems are raised soon after the commit, the code is
>>>>> left
>>>>>> untouched for a long time until it is work on again or simply is
>>>>> forgotten.*
>>>>>> {quote}
>>>>>>
>>>>>>
>>>>>> Best regards,
>>>>>>
>>>>>> Pierre Smits
>>>>>>
>>>>>> On Sat, Aug 18, 2018 at 10:52 AM, Suraj Khurana <
>>>>>> [hidden email]> wrote:
>>>>>>
>>>>>>> Hi Pierre,
>>>>>>>
>>>>>>> This is not a new patch, this is updated version of two years old patch
>>>>>>> which has been already reviewed if you follow comments on ticket.
>>>>>>> We need to add updated patch as well since many file path have been
>>>>> changed
>>>>>>> and we have data files refactoring as well.
>>>>>>>
>>>>>>> HTH.
>>>>>>> --
>>>>>>> Best Regards,
>>>>>>> Suraj Khurana | Omni-channel OMS Technical Expert
>>>>>>> HotWax Commerce  by  HotWax Systems
>>>>>>>
>>>>>>> On Sat, Aug 18, 2018 at 2:13 PM, Pierre Smits <[hidden email]>
>>>>>>> wrote:
>>>>>>>
>>>>>>>> Hi Suraj,
>>>>>>>>
>>>>>>>> Please revert! Within 10 minutes you posted a new patch and committed
>>>>> it
>>>>>>> to
>>>>>>>> trunk and closed the issue. It is customary to follow the 72 hr delay
>>>>>>> rule
>>>>>>>> to allow the community to review the changes and assess the impact.
>>>>>>>>
>>>>>>>>
>>>>>>>> Best regards,
>>>>>>>>
>>>>>>>> Pierre Smits
>>>>>>>>
>>>>>>>> Apache Trafodion <https://trafodion.apache.org>, Vice President
>>>>>>>> Apache Directory <https://directory.apache.org>, PMC Member
>>>>>>>> Apache Incubator <https://incubator.apache.org>, committer
>>>>>>>> Apache OFBiz <https://ofbiz.apache.org>, contributor since 2008
>>>>>>>> Apache Steve <https://steve.apache.org>, committer
>>>>>>>>
>>>>>>>>> On Sat, Aug 18, 2018 at 10:34 AM, <[hidden email]> wrote:
>>>>>>>>>
>>>>>>>>> Author: surajk
>>>>>>>>> Date: Sat Aug 18 08:34:18 2018
>>>>>>>>> New Revision: 1838320
>>>>>>>>>
>>>>>>>>> URL: http://svn.apache.org/viewvc?rev=1838320&view=rev
>>>>>>>>> Log:
>>>>>>>>> Improved: Added support to calculate deposit price as well while
>>>>>>> creating
>>>>>>>>> shopping cart item.
>>>>>>>>> (OFBIZ-7482)
>>>>>>>>>
>>>>>>>>> Modified:
>>>>>>>>> ofbiz/ofbiz-framework/trunk/applications/datamodel/data/
>>>>>>>>> seed/OrderSeedData.xml
>>>>>>>>> ofbiz/ofbiz-framework/trunk/applications/datamodel/data/
>>>>>>>>> seed/ProductSeedData.xml
>>>>>>>>> ofbiz/ofbiz-framework/trunk/applications/order/src/main/
>>>>>>>>> java/org/apache/ofbiz/order/shoppingcart/ShoppingCartItem.java
>>>>>>>>>
>>>>>>>>> Modified: ofbiz/ofbiz-framework/trunk/applications/datamodel/data/
>>>>>>>>> seed/OrderSeedData.xml
>>>>>>>>> URL: http://svn.apache.org/viewvc/ofbiz/ofbiz-framework/trunk/
>>>>>>>>> applications/datamodel/data/seed/OrderSeedData.xml?rev=
>>>>>>>>> 1838320&r1=1838319&r2=1838320&view=diff
>>>>>>>>> ============================================================
>>>>>>>>> ==================
>>>>>>>>> --- ofbiz/ofbiz-framework/trunk/applications/datamodel/data/
>>>>>>>> seed/OrderSeedData.xml
>>>>>>>>> (original)
>>>>>>>>> +++ ofbiz/ofbiz-framework/trunk/applications/datamodel/data/
>>>>>>>> seed/OrderSeedData.xml
>>>>>>>>> Sat Aug 18 08:34:18 2018
>>>>>>>>> @@ -49,6 +49,7 @@ under the License.
>>>>>>>>>       <OrderAdjustmentType description="Additional Feature" hasTable="N"
>>>>>>>>> orderAdjustmentTypeId="ADDITIONAL_FEATURE"/>
>>>>>>>>>       <OrderAdjustmentType description="Warranty" hasTable="N"
>>>>>>>>> orderAdjustmentTypeId="WARRANTY_ADJUSTMENT"/>
>>>>>>>>>       <OrderAdjustmentType description="Marketing Package Adjustment"
>>>>>>>>> hasTable="N" orderAdjustmentTypeId="MKTG_PKG_AUTO_ADJUST"/>
>>>>>>>>> +    <OrderAdjustmentType description="Deposit" hasTable="N"
>>>>>>>>> orderAdjustmentTypeId="DEPOSIT_ADJUSTMENT"/>
>>>>>>>>>
>>>>>>>>>       <OrderBlacklistType orderBlacklistTypeId="BLACKLIST_ADDRESS"
>>>>>>>>> description="Addresss"/>
>>>>>>>>>       <OrderBlacklistType orderBlacklistTypeId="BLACKLIST_CREDITCARD"
>>>>>>>>> description="Credit Card"/>
>>>>>>>>>
>>>>>>>>> Modified: ofbiz/ofbiz-framework/trunk/applications/datamodel/data/
>>>>>>>>> seed/ProductSeedData.xml
>>>>>>>>> URL: http://svn.apache.org/viewvc/ofbiz/ofbiz-framework/trunk/
>>>>>>>>> applications/datamodel/data/seed/ProductSeedData.xml?rev=
>>>>>>>>> 1838320&r1=1838319&r2=1838320&view=diff
>>>>>>>>> ============================================================
>>>>>>>>> ==================
>>>>>>>>> --- ofbiz/ofbiz-framework/trunk/applications/datamodel/data/
>>>>>>>> seed/ProductSeedData.xml
>>>>>>>>> (original)
>>>>>>>>> +++ ofbiz/ofbiz-framework/trunk/applications/datamodel/data/
>>>>>>>> seed/ProductSeedData.xml
>>>>>>>>> Sat Aug 18 08:34:18 2018
>>>>>>>>> @@ -281,6 +281,7 @@ under the License.
>>>>>>>>>       <ProductPriceType description="Minimum Order Price"
>>>>>>>>> productPriceTypeId="MINIMUM_ORDER_PRICE"/>
>>>>>>>>>       <ProductPriceType description="Shipping Allowance Price"
>>>>>>>>> productPriceTypeId="SHIPPING_ALLOWANCE"/>
>>>>>>>>>
>>>>>>>>> +    <ProductPricePurpose description="Deposit price"
>>>>>>>>> productPricePurposeId="DEPOSIT"/>
>>>>>>>>>       <ProductPricePurpose description="Purchase/Initial"
>>>>>>>>> productPricePurposeId="PURCHASE"/>
>>>>>>>>>       <ProductPricePurpose description="Recurring Charge"
>>>>>>>>> productPricePurposeId="RECURRING_CHARGE"/>
>>>>>>>>>       <ProductPricePurpose description="Usage Charge"
>>>>>>>>> productPricePurposeId="USAGE_CHARGE"/>
>>>>>>>>>
>>>>>>>>> Modified: ofbiz/ofbiz-framework/trunk/applications/order/src/main/
>>>>>>>>> java/org/apache/ofbiz/order/shoppingcart/ShoppingCartItem.java
>>>>>>>>> URL: http://svn.apache.org/viewvc/ofbiz/ofbiz-framework/trunk/
>>>>>>>>> applications/order/src/main/java/org/apache/ofbiz/order/
>>>>>>>>> shoppingcart/ShoppingCartItem.java?rev=1838320&r1=1838319&
>>>>>>>>> r2=1838320&view=diff
>>>>>>>>> ============================================================
>>>>>>>>> ==================
>>>>>>>>> --- ofbiz/ofbiz-framework/trunk/applications/order/src/main/
>>>>>>>>> java/org/apache/ofbiz/order/shoppingcart/ShoppingCartItem.java
>>>>>>>> (original)
>>>>>>>>> +++ ofbiz/ofbiz-framework/trunk/applications/order/src/main/
>>>>>>>>> java/org/apache/ofbiz/order/shoppingcart/ShoppingCartItem.java Sat
>>>>> Aug
>>>>>>>> 18
>>>>>>>>> 08:34:18 2018
>>>>>>>>> @@ -1038,6 +1038,7 @@ public class ShoppingCartItem implements
>>>>>>>>>               ProductPromoWorker.doPromotions(cart, dispatcher);
>>>>>>>>>           }
>>>>>>>>>
>>>>>>>>> +        calcDepositAdjustments();
>>>>>>>>>           if (!"PURCHASE_ORDER".equals(cart.getOrderType())) {
>>>>>>>>>               // store the auto-save cart
>>>>>>>>>               if (triggerExternalOps && ProductStoreWorker.
>>>>>>>> autoSaveCart(delegator,
>>>>>>>>> productStoreId)) {
>>>>>>>>> @@ -1061,6 +1062,33 @@ public class ShoppingCartItem implements
>>>>>>>>>           }
>>>>>>>>>       }
>>>>>>>>>
>>>>>>>>> +    public void calcDepositAdjustments() {
>>>>>>>>> +        List<GenericValue>itemAdjustments = this.getAdjustments();
>>>>>>>>> +        try {
>>>>>>>>> +            GenericValue depositAmount = EntityQuery.use(delegator).
>>>>>>>>> from("ProductPrice").where("productId", this.getProductId(),
>>>>>>>>> "productPricePurposeId", "DEPOSIT", "productPriceTypeId",
>>>>>>>>> "DEFAULT_PRICE").filterByDate().queryFirst();
>>>>>>>>> +            if (UtilValidate.isNotEmpty(depositAmount)) {
>>>>>>>>> +                Boolean updatedDepositAmount = false;
>>>>>>>>> +                BigDecimal adjustmentAmount =
>>>>>>>>> depositAmount.getBigDecimal("price").multiply(this.getQuantity(),
>>>>>>>>> generalRounding);
>>>>>>>>> +                // itemAdjustments is a reference so directly setting
>>>>>>>>> updated amount to the same.
>>>>>>>>> +                    for(GenericValue itemAdjustment :
>>>>>>> itemAdjustments) {
>>>>>>>>> + if("DEPOSIT_ADJUSTMENT".equals(itemAdjustment.
>>>>>>>>> getString("orderAdjustmentTypeId"))) {
>>>>>>>>> + itemAdjustment.set("amount",
>>>>>>>>> adjustmentAmount);
>>>>>>>>> +                            updatedDepositAmount = true;
>>>>>>>>> +                        }
>>>>>>>>> +                }
>>>>>>>>> +                if (!updatedDepositAmount) {
>>>>>>>>> +                    GenericValue orderAdjustment =
>>>>>>> delegator.makeValue("
>>>>>>>>> OrderAdjustment");
>>>>>>>>> + orderAdjustment.set("orderAdjustmentTypeId",
>>>>>>>>> "DEPOSIT_ADJUSTMENT");
>>>>>>>>> + orderAdjustment.set("description", "Surcharge
>>>>>>>>> Adjustment");
>>>>>>>>> +                    orderAdjustment.set("amount", adjustmentAmount);
>>>>>>>>> + this.addAdjustment(orderAdjustment);
>>>>>>>>> +                }
>>>>>>>>> +            }
>>>>>>>>> +        } catch (GenericEntityException e){
>>>>>>>>> +            Debug.logError("Error in fetching deposite price
>>>>>>> details!!",
>>>>>>>>> module);
>>>>>>>>> +        }
>>>>>>>>> +    }
>>>>>>>>> +
>>>>>>>>>       public void updatePrice(LocalDispatcher dispatcher, ShoppingCart
>>>>>>>>> cart) throws CartItemModifyException {
>>>>>>>>>           // set basePrice using the calculateProductPrice service
>>>>>>>>>           if (_product != null && isModifiedPrice == false) {
>>>>>>>>>
>>>>>>>>>
>>>>>>>>>
>>>