Re: svn commit: r1554265 - in /ofbiz/trunk/applications/order/src/org/ofbiz/order/shoppingcart: ShoppingCart.java ShoppingCartServices.java product/ProductPromoWorker.java

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

Re: svn commit: r1554265 - in /ofbiz/trunk/applications/order/src/org/ofbiz/order/shoppingcart: ShoppingCart.java ShoppingCartServices.java product/ProductPromoWorker.java

Adrian Crum-3
This commit breaks promotions.

1. Create a new sales order, customer DemoCustomer.
2. Add GZ-8544, qty 1 to the order.
3. Promo items do not appear in order.
4. Log shows an exception being thrown:

2014-01-14 12:15:56,030 (http-bio-0.0.0.0-8443-exec-1) [  
ProductPromoWorker.java:385:ERROR]
---- runtime exception report  
--------------------------------------------------
Error running promotions, will ignore: java.lang.ArithmeticException:  
Non-terminating decimal expansion; no exact representable decimal  
result.
Exception: java.lang.ArithmeticException
Message: Non-terminating decimal expansion; no exact representable  
decimal result.
---- stack trace  
---------------------------------------------------------------
java.lang.ArithmeticException: Non-terminating decimal expansion; no  
exact representable decimal result.
java.math.BigDecimal.divide(BigDecimal.java:1616)
org.ofbiz.order.shoppingcart.ShoppingCart$ProductPromoUseInfo.getUsageWeight(ShoppingCart.java:4418)
org.ofbiz.order.shoppingcart.ShoppingCart$ProductPromoUseInfo.compareTo(ShoppingCart.java:4423)
org.ofbiz.order.shoppingcart.ShoppingCart$ProductPromoUseInfo.compareTo(ShoppingCart.java:4388)
java.util.ComparableTimSort.countRunAndMakeAscending(ComparableTimSort.java:290)
java.util.ComparableTimSort.sort(ComparableTimSort.java:157)
java.util.ComparableTimSort.sort(ComparableTimSort.java:146)
java.util.Arrays.sort(Arrays.java:472)
java.util.Collections.sort(Collections.java:155)
org.ofbiz.order.shoppingcart.product.ProductPromoWorker.doPromotions(ProductPromoWorker.java:334)
org.ofbiz.order.shoppingcart.product.ProductPromoWorker.doPromotions(ProductPromoWorker.java:293)
org.ofbiz.order.shoppingcart.ShoppingCartItem.setQuantity(ShoppingCartItem.java:1061)
org.ofbiz.order.shoppingcart.ShoppingCartItem.makeItem(ShoppingCartItem.java:502)
org.ofbiz.order.shoppingcart.ShoppingCartItem.makeItem(ShoppingCartItem.java:334)
org.ofbiz.order.shoppingcart.ShoppingCart.addOrIncreaseItem(ShoppingCart.java:590)
org.ofbiz.order.shoppingcart.ShoppingCartHelper.addToCart(ShoppingCartHelper.java:245)
org.ofbiz.order.shoppingcart.ShoppingCartEvents.addToCart(ShoppingCartEvents.java:638)
sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:57)
sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
java.lang.reflect.Method.invoke(Method.java:606)
org.ofbiz.webapp.event.JavaEventHandler.invoke(JavaEventHandler.java:93)
org.ofbiz.webapp.event.JavaEventHandler.invoke(JavaEventHandler.java:79)
org.ofbiz.webapp.control.RequestHandler.runEvent(RequestHandler.java:749)
org.ofbiz.webapp.control.RequestHandler.doRequest(RequestHandler.java:469)
org.ofbiz.webapp.control.ControlServlet.doGet(ControlServlet.java:219)
org.ofbiz.webapp.control.ControlServlet.doPost(ControlServlet.java:91)
javax.servlet.http.HttpServlet.service(HttpServlet.java:641)
javax.servlet.http.HttpServlet.service(HttpServlet.java:722)
org.apache.catalina.core.ApplicationFilterChain.internalDoFilter(ApplicationFilterChain.java:305)
org.apache.catalina.core.ApplicationFilterChain.doFilter(ApplicationFilterChain.java:210)
org.ofbiz.webapp.control.ContextFilter.doFilter(ContextFilter.java:314)
org.apache.catalina.core.ApplicationFilterChain.internalDoFilter(ApplicationFilterChain.java:243)
org.apache.catalina.core.ApplicationFilterChain.doFilter(ApplicationFilterChain.java:210)
org.apache.catalina.core.StandardWrapperValve.invoke(StandardWrapperValve.java:222)
org.apache.catalina.core.StandardContextValve.invoke(StandardContextValve.java:123)
org.apache.catalina.authenticator.AuthenticatorBase.invoke(AuthenticatorBase.java:502)
org.apache.catalina.core.StandardHostValve.invoke(StandardHostValve.java:171)
org.apache.catalina.valves.ErrorReportValve.invoke(ErrorReportValve.java:100)
org.apache.catalina.core.StandardEngineValve.invoke(StandardEngineValve.java:118)
org.apache.catalina.valves.AccessLogValve.invoke(AccessLogValve.java:953)
org.apache.catalina.connector.CoyoteAdapter.service(CoyoteAdapter.java:409)
org.apache.coyote.http11.AbstractHttp11Processor.process(AbstractHttp11Processor.java:1044)
org.apache.coyote.AbstractProtocol$AbstractConnectionHandler.process(AbstractProtocol.java:607)
org.apache.tomcat.util.net.JIoEndpoint$SocketProcessor.run(JIoEndpoint.java:315)
java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1145)
java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:615)
java.lang.Thread.run(Thread.java:744)
--------------------------------------------------------------------------------


-Adrian



Quoting [hidden email]:

> Author: jacopoc
> Date: Mon Dec 30 15:53:07 2013
> New Revision: 1554265
>
> URL: http://svn.apache.org/r1554265
> Log:
> This is a refactoring of the product promotion engine in order to  
> overcome some limitations that prevented it to select and apply the  
> best set of promotions under special conditions.
>
> Example: Consider two promotions:
> * consider two products: Product A, with price $20, and Product B,  
> with price $40
> * Promotion 1: 20% discount on all the products in a category  
> containing Product A and Product B
> * Promotion 2: 40% discount on Product A
>
> When Product A and Product B are both in the cart:
> * Expected behavior: on Product A the Promotion 2 should be applied  
> i.e. 40% discount, and on Product B Promotion 1 should be applied  
> i.e. 20% discount.
> ** Summary
> Product Price Discount Subtotal
> A $20 $8 (40% discount) $12
> B $40 $8 (20% discount) $32
> Total Adjustment: $16
>
> * OFBiz behavior (before this fix): Promotion 1 is applied to  
> Product A and Product B; this happens because the total discount of  
> Promotion 1 is greater than the total discount of Promotion 2 and  
> OFBiz applies promotions sorted by discount (desc)
> ** Summary
> Product Price Discount Subtotal
> A $20 $4 (20% discount) $16
> B $40 $8 (20% discount) $32
> Total Adjustment: $12
>
> The new solution fixes this issue and similar ones.
>
> Here are some details about the new algorithm.
>
> Overview of the flow:
> 1) run the promotions one by one in a test run
> 2) collect the ProductPromoUse information
> 3) sort them by weight (i.e. the ratio between the discount and the  
> value of the products discounted)
> 4) execute the ProductPromoUse in the given order
>
> In order to understand this solution, and specifically the changes  
> to ProductPromoWorker.java, there is an important concept to consider:
> one Promotion can generate more than one ProductPromoUseInfo objects.
> For example if I have 2 units of WG-1111 in the cart (in one cart  
> item) and I have the promotion “20% discount on WG-1111 and GZ-1000”  
> then the system will create TWO ProductPromoUseInfo objects both  
> associated to the same promotion one for each of the 2 units  
> discounted.
> Similarly if I had two lines: 2 units of WG-1111 and 1 unit of  
> GZ-1000 I would get 3 ProductPromoUseInfo objects 2 objects for  
> WG-1111 and 1 object for GZ-1000
>
> We can sort these ProductPromoUseInfo objects based on their weight  
> (i.e. the ratio between the discount and the value of the products  
> discounted) in desc order
> and now we have a sorted list of ProductPromoUseInfo objects ready  
> to be executed
> However we only want to execute each of them once and for this  
> reason we set (in memory, not in the DB) the useLimitPerOrder to 1  
> in the first ProductPromoUseInfo of a given promotion and then to 2  
> if the same ProductPromoUseInfo is associated to the same promotion  
> etc...
> in this way the end result is that the system will generate, as we  
> desire, ONE ProductPromoUseInfo only for each of the  
> ProductPromoUseInfo in the list.
>
> Here is an example:
> we have 2 promotions:
> PROMO A
> PROMO B
>
> After test run:
>
> ProductPromoUseInfo - PROMO A - #1 - weight 0.3
> ProductPromoUseInfo - PROMO A - #2 - weight 0.3
> ProductPromoUseInfo - PROMO B - #1 - weight 0.4
>
> After sorting:
>
> ProductPromoUseInfo - PROMO B - #1 - weight 0.4
> ProductPromoUseInfo - PROMO A - #1 - weight 0.3
> ProductPromoUseInfo - PROMO A - #2 - weight 0.3
>
> Based on this we create a list (sortedExplodedProductPromoList) of  
> ProductPromo:
>
> PROMO B - with useLimitPerOrder=1
> PROMO A - with useLimitPerOrder=1
> PROMO A - with useLimitPerOrder=2
>
> When we apply these to the cart we get the following results:
>
> PROMO B - with useLimitPerOrder=1 APPLIED
> PROMO A - with useLimitPerOrder=1 APPLIED
> PROMO A - with useLimitPerOrder=2 NOT APPLIED (because PROMO B used the item)
>
>
> Modified:
>      
> ofbiz/trunk/applications/order/src/org/ofbiz/order/shoppingcart/ShoppingCart.java
>      
> ofbiz/trunk/applications/order/src/org/ofbiz/order/shoppingcart/ShoppingCartServices.java
>      
> ofbiz/trunk/applications/order/src/org/ofbiz/order/shoppingcart/product/ProductPromoWorker.java
>
> Modified:  
> ofbiz/trunk/applications/order/src/org/ofbiz/order/shoppingcart/ShoppingCart.java
> URL:  
> http://svn.apache.org/viewvc/ofbiz/trunk/applications/order/src/org/ofbiz/order/shoppingcart/ShoppingCart.java?rev=1554265&r1=1554264&r2=1554265&view=diff
> ==============================================================================
> ---  
> ofbiz/trunk/applications/order/src/org/ofbiz/order/shoppingcart/ShoppingCart.java  
> (original)
> +++  
> ofbiz/trunk/applications/order/src/org/ofbiz/order/shoppingcart/ShoppingCart.java Mon Dec 30 15:53:07  
> 2013
> @@ -2714,7 +2714,7 @@ public class ShoppingCart implements Ite
>              }
>              itemsTotal = itemsTotal.add(cartItem.getItemSubTotal());
>          }
> -        return itemsTotal;
> +        return itemsTotal.add(this.getOrderOtherAdjustmentTotal());
>      }
>
>      /**
> @@ -3142,12 +3142,12 @@ public class ShoppingCart implements Ite
>          return new HashMap<GenericPK,  
> String>(this.desiredAlternateGiftByAction);
>      }
>
> -    public void addProductPromoUse(String productPromoId, String  
> productPromoCodeId, BigDecimal totalDiscountAmount, BigDecimal  
> quantityLeftInActions) {
> +    public void addProductPromoUse(String productPromoId, String  
> productPromoCodeId, BigDecimal totalDiscountAmount, BigDecimal  
> quantityLeftInActions, Map<ShoppingCartItem,BigDecimal>  
> usageInfoMap) {
>          if (UtilValidate.isNotEmpty(productPromoCodeId) &&  
> !this.productPromoCodes.contains(productPromoCodeId)) {
>              throw new IllegalStateException("Cannot add a use to a  
> promo code use for a code that has not been entered.");
>          }
>          if (Debug.verboseOn()) Debug.logVerbose("Used promotion ["  
> + productPromoId + "] with code [" + productPromoCodeId + "] for  
> total discount [" + totalDiscountAmount + "] and quantity left in  
> actions [" + quantityLeftInActions + "]", module);
> -        this.productPromoUseInfoList.add(new  
> ProductPromoUseInfo(productPromoId, productPromoCodeId,  
> totalDiscountAmount, quantityLeftInActions));
> +        this.productPromoUseInfoList.add(new  
> ProductPromoUseInfo(productPromoId, productPromoCodeId,  
> totalDiscountAmount, quantityLeftInActions, usageInfoMap));
>      }
>
>      public void removeProductPromoUse(String productPromoId) {
> @@ -4385,23 +4385,43 @@ public class ShoppingCart implements Ite
>          }
>      }
>
> -    public static class ProductPromoUseInfo implements Serializable {
> +    public static class ProductPromoUseInfo implements  
> Serializable, Comparable<ProductPromoUseInfo> {
>          public String productPromoId = null;
>          public String productPromoCodeId = null;
>          public BigDecimal totalDiscountAmount = BigDecimal.ZERO;
>          public BigDecimal quantityLeftInActions = BigDecimal.ZERO;
> +        private Map<ShoppingCartItem,BigDecimal> usageInfoMap = null;
>
> -        public ProductPromoUseInfo(String productPromoId, String  
> productPromoCodeId, BigDecimal totalDiscountAmount, BigDecimal  
> quantityLeftInActions) {
> +        public ProductPromoUseInfo(String productPromoId, String  
> productPromoCodeId, BigDecimal totalDiscountAmount, BigDecimal  
> quantityLeftInActions, Map<ShoppingCartItem,BigDecimal>  
> usageInfoMap) {
>              this.productPromoId = productPromoId;
>              this.productPromoCodeId = productPromoCodeId;
>              this.totalDiscountAmount = totalDiscountAmount;
>              this.quantityLeftInActions = quantityLeftInActions;
> +            this.usageInfoMap = usageInfoMap;
>          }
>
>          public String getProductPromoId() { return this.productPromoId; }
>          public String getProductPromoCodeId() { return  
> this.productPromoCodeId; }
>          public BigDecimal getTotalDiscountAmount() { return  
> this.totalDiscountAmount; }
>          public BigDecimal getQuantityLeftInActions() { return  
> this.quantityLeftInActions; }
> +        public Map<ShoppingCartItem,BigDecimal> getUsageInfoMap() {  
> return this.usageInfoMap; }
> +        public BigDecimal getUsageWeight() {
> +            Iterator<ShoppingCartItem> lineItems =  
> this.usageInfoMap.keySet().iterator();
> +            BigDecimal totalAmount = BigDecimal.ZERO;
> +            while (lineItems.hasNext()) {
> +                ShoppingCartItem lineItem = lineItems.next();
> +                totalAmount =  
> totalAmount.add(lineItem.getBasePrice().multiply(usageInfoMap.get(lineItem)));
> +            }
> +            if (totalAmount.compareTo(BigDecimal.ZERO) == 0) {
> +                return BigDecimal.ZERO;
> +            } else {
> +                return  
> getTotalDiscountAmount().negate().divide(totalAmount);
> +            }
> +        }
> +
> +        public int compareTo(ProductPromoUseInfo other) {
> +            return other.getUsageWeight().compareTo(getUsageWeight());
> +        }
>      }
>
>      public static class CartShipInfo implements Serializable {
>
> Modified:  
> ofbiz/trunk/applications/order/src/org/ofbiz/order/shoppingcart/ShoppingCartServices.java
> URL:  
> http://svn.apache.org/viewvc/ofbiz/trunk/applications/order/src/org/ofbiz/order/shoppingcart/ShoppingCartServices.java?rev=1554265&r1=1554264&r2=1554265&view=diff
> ==============================================================================
> ---  
> ofbiz/trunk/applications/order/src/org/ofbiz/order/shoppingcart/ShoppingCartServices.java  
> (original)
> +++  
> ofbiz/trunk/applications/order/src/org/ofbiz/order/shoppingcart/ShoppingCartServices.java Mon Dec 30 15:53:07  
> 2013
> @@ -21,6 +21,7 @@ package org.ofbiz.order.shoppingcart;
>  import java.math.BigDecimal;
>  import java.math.MathContext;
>  import java.sql.Timestamp;
> +import java.util.HashMap;
>  import java.util.Iterator;
>  import java.util.List;
>  import java.util.Locale;
> @@ -629,7 +630,7 @@ public class ShoppingCartServices {
>                  cart.addProductPromoCode(productPromoCode, dispatcher);
>              }
>              for (GenericValue productPromoUse: orh.getProductPromoUse()) {
> -                
> cart.addProductPromoUse(productPromoUse.getString("productPromoId"),  
> productPromoUse.getString("productPromoCodeId"),  
> productPromoUse.getBigDecimal("totalDiscountAmount"),  
> productPromoUse.getBigDecimal("quantityLeftInActions"));
> +                
> cart.addProductPromoUse(productPromoUse.getString("productPromoId"),  
> productPromoUse.getString("productPromoCodeId"),  
> productPromoUse.getBigDecimal("totalDiscountAmount"),  
> productPromoUse.getBigDecimal("quantityLeftInActions"), new  
> HashMap<ShoppingCartItem, BigDecimal>());
>              }
>          }
>
>
> Modified:  
> ofbiz/trunk/applications/order/src/org/ofbiz/order/shoppingcart/product/ProductPromoWorker.java
> URL:  
> http://svn.apache.org/viewvc/ofbiz/trunk/applications/order/src/org/ofbiz/order/shoppingcart/product/ProductPromoWorker.java?rev=1554265&r1=1554264&r2=1554265&view=diff
> ==============================================================================
> ---  
> ofbiz/trunk/applications/order/src/org/ofbiz/order/shoppingcart/product/ProductPromoWorker.java  
> (original)
> +++  
> ofbiz/trunk/applications/order/src/org/ofbiz/order/shoppingcart/product/ProductPromoWorker.java Mon Dec 30 15:53:07  
> 2013
> @@ -22,6 +22,8 @@ import java.math.BigDecimal;
>  import java.math.MathContext;
>  import java.sql.Timestamp;
>  import java.util.ArrayList;
> +import java.util.Collections;
> +import java.util.HashMap;
>  import java.util.Iterator;
>  import java.util.List;
>  import java.util.Locale;
> @@ -51,6 +53,7 @@ import org.ofbiz.entity.condition.Entity
>  import org.ofbiz.entity.util.EntityUtil;
>  import org.ofbiz.order.shoppingcart.CartItemModifyException;
>  import org.ofbiz.order.shoppingcart.ShoppingCart;
> +import org.ofbiz.order.shoppingcart.ShoppingCart.ProductPromoUseInfo;
>  import org.ofbiz.order.shoppingcart.ShoppingCartEvents;
>  import org.ofbiz.order.shoppingcart.ShoppingCartItem;
>  import org.ofbiz.product.product.ProductContentWrapper;
> @@ -318,44 +321,62 @@ public class ProductPromoWorker {
>              // do a calculate only run through the promotions, then  
> order by descending totalDiscountAmount for each promotion
>              // NOTE: on this run, with isolatedTestRun passed as  
> false it should not apply any adjustments
>              //  or track which cart items are used for which  
> promotions, but it will track ProductPromoUseInfo and
> -            //  useLimits; we are basicly just trying to run each  
> promo "independently" to see how much each is worth
> +            //  useLimits; we are basically just trying to run each  
> promo "independently" to see how much each is worth
>              runProductPromos(productPromoList, cart, delegator,  
> dispatcher, nowTimestamp, true);
>
> -            // NOTE: after that first pass we could remove any that  
> have a 0 totalDiscountAmount from the run list, but we won't because  
> by the time they are run the cart may have changed enough to get  
> them to go; also, certain actions like free shipping should always  
> be run even though we won't know what the totalDiscountAmount is at  
> the time the promotion is run
> -            // each ProductPromoUseInfo on the shopping cart will  
> contain it's total value, so add up all totals for each promoId and  
> put them in a List of Maps
> -            // create a List of Maps with productPromo and  
> totalDiscountAmount, use the Map sorter to sort them descending by  
> totalDiscountAmount
> -
> -            // before sorting split into two lists and sort each  
> list; one list for promos that have a order total condition, and the  
> other list for all promos that don't; then we'll always run the ones  
> that have no condition on the order total first
> -            List<Map<Object, Object>> productPromoDiscountMapList =  
> FastList.newInstance();
> -            List<Map<Object, Object>>  
> productPromoDiscountMapListOrderTotal = FastList.newInstance();
> +            // NOTE: we can easily recognize the promos for the  
> order total: they are the ones with usage set to 0
> +            Iterator<ProductPromoUseInfo> promoUses =  
> cart.getProductPromoUseInfoIter();
> +            List<ProductPromoUseInfo> sortedPromoUses = new  
> ArrayList<ProductPromoUseInfo>();
> +            while (promoUses.hasNext()) {
> +                ProductPromoUseInfo promoUse = promoUses.next();
> +                sortedPromoUses.add(promoUse);
> +            }
> +            Collections.sort(sortedPromoUses);
> +            List<GenericValue> sortedExplodedProductPromoList = new  
> ArrayList<GenericValue>(sortedPromoUses.size());
> +            Map<String, Long> usesPerPromo = new HashMap<String, Long>();
> +            int indexOfFirstOrderTotalPromo = -1;
> +            for (ProductPromoUseInfo promoUse: sortedPromoUses) {
> +                GenericValue productPromo =  
> delegator.findOne("ProductPromo", UtilMisc.toMap("productPromoId",  
> promoUse.getProductPromoId()), true);
> +                GenericValue newProductPromo =  
> (GenericValue)productPromo.clone();
> +                if  
> (!usesPerPromo.containsKey(promoUse.getProductPromoId())) {
> +                    usesPerPromo.put(promoUse.getProductPromoId(), 0l);
> +                }
> +                long uses = usesPerPromo.get(promoUse.getProductPromoId());
> +                uses = uses + 1;
> +                long useLimitPerOrder =  
> (newProductPromo.get("useLimitPerOrder") != null?  
> newProductPromo.getLong("useLimitPerOrder"): -1);
> +                if (useLimitPerOrder == -1 || uses < useLimitPerOrder) {
> +                    newProductPromo.set("useLimitPerOrder", uses);
> +                }
> +                usesPerPromo.put(promoUse.getProductPromoId(), uses);
> +                sortedExplodedProductPromoList.add(newProductPromo);
> +                if (indexOfFirstOrderTotalPromo == -1 &&  
> BigDecimal.ZERO.equals(promoUse.getUsageWeight())) {
> +                    indexOfFirstOrderTotalPromo =  
> sortedExplodedProductPromoList.size() - 1;
> +                }
> +            }
> +            if (indexOfFirstOrderTotalPromo == -1) {
> +                indexOfFirstOrderTotalPromo =  
> sortedExplodedProductPromoList.size() - 1;
> +            }
> +
>              for (GenericValue productPromo : productPromoList) {
>                  Map<Object, Object> productPromoDiscountMap =  
> UtilGenerics.checkMap(UtilMisc.toMap("productPromo", productPromo,  
> "totalDiscountAmount",  
> cart.getProductPromoUseTotalDiscount(productPromo.getString("productPromoId"))));
>                  if (hasOrderTotalCondition(productPromo, delegator)) {
> -                    
> productPromoDiscountMapListOrderTotal.add(productPromoDiscountMap);
> +                    if  
> (!usesPerPromo.containsKey(productPromo.getString("productPromoId")))  
> {
> +                        sortedExplodedProductPromoList.add(productPromo);
> +                    }
>                  } else {
> -                    
> productPromoDiscountMapList.add(productPromoDiscountMap);
> +                    if  
> (!usesPerPromo.containsKey(productPromo.getString("productPromoId")))  
> {
> +                        if (indexOfFirstOrderTotalPromo != -1) {
> +                            
> sortedExplodedProductPromoList.add(indexOfFirstOrderTotalPromo,  
> productPromo);
> +                        } else {
> +                            sortedExplodedProductPromoList.add(0,  
> productPromo);
> +                        }
> +                    }
>                  }
>              }
>
> -
> -            // sort the Map List, do it ascending because the  
> discount amounts will be negative, so the lowest number is really  
> the highest discount
> -            productPromoDiscountMapList =  
> UtilMisc.sortMaps(productPromoDiscountMapList,  
> UtilMisc.toList("+totalDiscountAmount"));
> -            productPromoDiscountMapListOrderTotal =  
> UtilMisc.sortMaps(productPromoDiscountMapListOrderTotal,  
> UtilMisc.toList("+totalDiscountAmount"));
> -
> -            
> productPromoDiscountMapList.addAll(productPromoDiscountMapListOrderTotal);
> -
> -            List<GenericValue> sortedProductPromoList = new  
> ArrayList<GenericValue>(productPromoDiscountMapList.size());
> -            Iterator<Map<Object, Object>>  
> productPromoDiscountMapIter = productPromoDiscountMapList.iterator();
> -            while (productPromoDiscountMapIter.hasNext()) {
> -                Map<Object, Object> productPromoDiscountMap =  
> UtilGenerics.checkMap(productPromoDiscountMapIter.next());
> -                GenericValue productPromo = (GenericValue)  
> productPromoDiscountMap.get("productPromo");
> -                sortedProductPromoList.add(productPromo);
> -                if (Debug.verboseOn()) Debug.logVerbose("Sorted  
> Promo [" + productPromo.getString("productPromoId") + "] with total  
> discount: " + productPromoDiscountMap.get("totalDiscountAmount"),  
> module);
> -            }
> -
>              // okay, all ready, do the real run, clearing the  
> temporary result first...
>              cart.clearAllPromotionInformation();
> -            runProductPromos(sortedProductPromoList, cart,  
> delegator, dispatcher, nowTimestamp, false);
> +            runProductPromos(sortedExplodedProductPromoList, cart,  
> delegator, dispatcher, nowTimestamp, false);
>          } catch (NumberFormatException e) {
>              Debug.logError(e, "Number not formatted correctly in  
> promotion rules, not completed...", module);
>          } catch (GenericEntityException e) {
> @@ -436,7 +457,7 @@ public class ProductPromoWorker {
>                                      GenericValue productPromoCode =  
> productPromoCodeIter.next();
>                                      String productPromoCodeId =  
> productPromoCode.getString("productPromoCodeId");
>                                      Long codeUseLimit =  
> getProductPromoCodeUseLimit(productPromoCode, partyId, delegator);
> -                                    if (runProductPromoRules(cart,  
> cartChanged, useLimit, true, productPromoCodeId, codeUseLimit,  
> maxUseLimit, productPromo, productPromoRules, dispatcher, delegator,  
> nowTimestamp)) {
> +                                    if (runProductPromoRules(cart,  
> useLimit, true, productPromoCodeId, codeUseLimit, maxUseLimit,  
> productPromo, productPromoRules, dispatcher, delegator,  
> nowTimestamp)) {
>                                          cartChanged = true;
>                                      }
>
> @@ -448,7 +469,7 @@ public class ProductPromoWorker {
>                              }
>                          } else {
>                              try {
> -                                if (runProductPromoRules(cart,  
> cartChanged, useLimit, false, null, null, maxUseLimit, productPromo,  
> productPromoRules, dispatcher, delegator, nowTimestamp)) {
> +                                if (runProductPromoRules(cart,  
> useLimit, false, null, null, maxUseLimit, productPromo,  
> productPromoRules, dispatcher, delegator, nowTimestamp)) {
>                                      cartChanged = true;
>                                  }
>                              } catch (RuntimeException e) {
> @@ -735,8 +756,10 @@ public class ProductPromoWorker {
>          return promoDescBuf.toString();
>      }
>
> -    protected static boolean runProductPromoRules(ShoppingCart  
> cart, boolean cartChanged, Long useLimit, boolean requireCode,  
> String productPromoCodeId, Long codeUseLimit, long maxUseLimit,
> +    protected static boolean runProductPromoRules(ShoppingCart  
> cart, Long useLimit, boolean requireCode, String productPromoCodeId,  
> Long codeUseLimit, long maxUseLimit,
>          GenericValue productPromo, List<GenericValue>  
> productPromoRules, LocalDispatcher dispatcher, Delegator delegator,  
> Timestamp nowTimestamp) throws GenericEntityException,  
> UseLimitException {
> +        boolean cartChanged = false;
> +        Map<ShoppingCartItem,BigDecimal> usageInfoMap =  
> prepareProductUsageInfoMap(cart);
>          String productPromoId = productPromo.getString("productPromoId");
>          while ((useLimit == null || useLimit.longValue() >  
> cart.getProductPromoUseCount(productPromoId)) &&
>                  (!requireCode ||  
> UtilValidate.isNotEmpty(productPromoCodeId)) &&
> @@ -755,17 +778,17 @@ public class ProductPromoWorker {
>                  // loop through conditions for rule, if any false,  
> set allConditionsTrue to false
>                  List<GenericValue> productPromoConds =  
> delegator.findByAnd("ProductPromoCond",  
> UtilMisc.toMap("productPromoId",  
> productPromo.get("productPromoId")),  
> UtilMisc.toList("productPromoCondSeqId"), true);
>                  productPromoConds =  
> EntityUtil.filterByAnd(productPromoConds,  
> UtilMisc.toMap("productPromoRuleId",  
> productPromoRule.get("productPromoRuleId")));
> -                // using the other method to consolodate cache  
> entries because the same cache is used elsewhere: List  
> productPromoConds = productPromoRule.getRelated("ProductPromoCond",  
> null, UtilMisc.toList("productPromoCondSeqId"), true);
> +                // using the other method to consolidate cache  
> entries because the same cache is used elsewhere: List  
> productPromoConds = productPromoRule.getRelated("ProductPromoCond",  
> null, UtilMisc.toList("productPromoCondSeqId"), true);
>                  if (Debug.verboseOn()) Debug.logVerbose("Checking "  
> + productPromoConds.size() + " conditions for rule " +  
> productPromoRule, module);
>
>                  Iterator<GenericValue> productPromoCondIter =  
> UtilMisc.toIterator(productPromoConds);
>                  while (productPromoCondIter != null &&  
> productPromoCondIter.hasNext()) {
>                      GenericValue productPromoCond =  
> productPromoCondIter.next();
>
> -                    boolean condResult =  
> checkCondition(productPromoCond, cart, delegator, dispatcher,  
> nowTimestamp);
> +                    boolean conditionSatisfied =  
> checkCondition(productPromoCond, cart, delegator, dispatcher,  
> nowTimestamp);
>
>                      // any false condition will cause it to NOT  
> perform the action
> -                    if (condResult == false) {
> +                    if (!conditionSatisfied) {
>                          performActions = false;
>                          break;
>                      }
> @@ -797,13 +820,16 @@ public class ProductPromoWorker {
>              }
>
>              if (promoUsed) {
> -                
> cart.addProductPromoUse(productPromo.getString("productPromoId"),  
> productPromoCodeId, totalDiscountAmount, quantityLeftInActions);
> +                // Get product use information from the cart
> +                Map<ShoppingCartItem,BigDecimal> newUsageInfoMap =  
> prepareProductUsageInfoMap(cart);
> +                Map<ShoppingCartItem,BigDecimal> deltaUsageInfoMap  
> = prepareDeltaProductUsageInfoMap(usageInfoMap, newUsageInfoMap);
> +                usageInfoMap = newUsageInfoMap;
> +                
> cart.addProductPromoUse(productPromo.getString("productPromoId"),  
> productPromoCodeId, totalDiscountAmount, quantityLeftInActions,  
> deltaUsageInfoMap);
>              } else {
>                  // the promotion was not used, don't try again  
> until we finish a full pass and come back to see the promo  
> conditions are now satisfied based on changes to the cart
>                  break;
>              }
>
> -
>              if (cart.getProductPromoUseCount(productPromoId) >  
> maxUseLimit) {
>                  throw new UseLimitException("ERROR: While  
> calculating promotions the promotion [" + productPromoId + "] action  
> was applied more than " + maxUseLimit + " times, so the calculation  
> has been ended. This should generally never happen unless you have  
> bad rule definitions.");
>              }
> @@ -812,6 +838,34 @@ public class ProductPromoWorker {
>          return cartChanged;
>      }
>
> +    private static Map<ShoppingCartItem,BigDecimal>  
> prepareProductUsageInfoMap(ShoppingCart cart) {
> +        Map<ShoppingCartItem,BigDecimal> usageInfoMap = new  
> HashMap<ShoppingCartItem, BigDecimal>();
> +        List<ShoppingCartItem> lineOrderedByBasePriceList =  
> cart.getLineListOrderedByBasePrice(false);
> +        for (ShoppingCartItem cartItem : lineOrderedByBasePriceList) {
> +            BigDecimal used = cartItem.getPromoQuantityUsed();
> +            if (used.compareTo(BigDecimal.ZERO) != 0) {
> +                usageInfoMap.put(cartItem, used);
> +            }
> +        }
> +        return usageInfoMap;
> +    }
> +
> +    private static Map<ShoppingCartItem,BigDecimal>  
> prepareDeltaProductUsageInfoMap(Map<ShoppingCartItem,BigDecimal>  
> oldMap, Map<ShoppingCartItem,BigDecimal> newMap) {
> +        Map<ShoppingCartItem,BigDecimal> deltaUsageInfoMap = new  
> HashMap<ShoppingCartItem, BigDecimal>(newMap);
> +        Iterator<ShoppingCartItem> cartLines = oldMap.keySet().iterator();
> +        while (cartLines.hasNext()) {
> +            ShoppingCartItem cartLine = cartLines.next();
> +            BigDecimal oldUsed = oldMap.get(cartLine);
> +            BigDecimal newUsed = newMap.get(cartLine);
> +            if (newUsed.compareTo(oldUsed) > 0) {
> +                deltaUsageInfoMap.put(cartLine,  
> newUsed.add(oldUsed.negate()));
> +            } else {
> +                deltaUsageInfoMap.remove(cartLine);
> +            }
> +        }
> +        return deltaUsageInfoMap;
> +    }
> +
>      protected static boolean checkCondition(GenericValue  
> productPromoCond, ShoppingCart cart, Delegator delegator,  
> LocalDispatcher dispatcher, Timestamp nowTimestamp) throws  
> GenericEntityException {
>          String condValue = productPromoCond.getString("condValue");
>          String otherValue = productPromoCond.getString("otherValue");
> @@ -1772,8 +1826,8 @@ public class ProductPromoWorker {
>              actionResultInfo.ranAction = false;
>          }
>
> +        // in action, if doesn't have enough quantity to use the  
> promo at all, remove candidate promo uses and increment  
> promoQuantityUsed; this should go for all actions, if any action  
> runs we confirm
>          if (actionResultInfo.ranAction) {
> -            // in action, if doesn't have enough quantity to use  
> the promo at all, remove candidate promo uses and increment  
> promoQuantityUsed; this should go for all actions, if any action  
> runs we confirm
>              
> cart.confirmPromoRuleUse(productPromoAction.getString("productPromoId"),  
> productPromoAction.getString("productPromoRuleId"));
>          } else {
>              
> cart.resetPromoRuleUse(productPromoAction.getString("productPromoId"),  
> productPromoAction.getString("productPromoRuleId"));
>
>
>



Reply | Threaded
Open this post in threaded view
|

Re: svn commit: r1554265 - in /ofbiz/trunk/applications/order/src/org/ofbiz/order/shoppingcart: ShoppingCart.java ShoppingCartServices.java product/ProductPromoWorker.java

Jacopo Cappellato-4
Thanks Adrian,

I am looking into this.

Jacopo

On Jan 14, 2014, at 6:24 PM, [hidden email] wrote:

> This commit breaks promotions.
>
> 1. Create a new sales order, customer DemoCustomer.
> 2. Add GZ-8544, qty 1 to the order.
> 3. Promo items do not appear in order.
> 4. Log shows an exception being thrown:
>
> 2014-01-14 12:15:56,030 (http-bio-0.0.0.0-8443-exec-1) [ ProductPromoWorker.java:385:ERROR]
> ---- runtime exception report --------------------------------------------------
> Error running promotions, will ignore: java.lang.ArithmeticException: Non-terminating decimal expansion; no exact representable decimal result.
> Exception: java.lang.ArithmeticException
> Message: Non-terminating decimal expansion; no exact representable decimal result.
> ---- stack trace ---------------------------------------------------------------
> java.lang.ArithmeticException: Non-terminating decimal expansion; no exact representable decimal result.
> java.math.BigDecimal.divide(BigDecimal.java:1616)
> org.ofbiz.order.shoppingcart.ShoppingCart$ProductPromoUseInfo.getUsageWeight(ShoppingCart.java:4418)
> org.ofbiz.order.shoppingcart.ShoppingCart$ProductPromoUseInfo.compareTo(ShoppingCart.java:4423)
> org.ofbiz.order.shoppingcart.ShoppingCart$ProductPromoUseInfo.compareTo(ShoppingCart.java:4388)
> java.util.ComparableTimSort.countRunAndMakeAscending(ComparableTimSort.java:290)
> java.util.ComparableTimSort.sort(ComparableTimSort.java:157)
> java.util.ComparableTimSort.sort(ComparableTimSort.java:146)
> java.util.Arrays.sort(Arrays.java:472)
> java.util.Collections.sort(Collections.java:155)
> org.ofbiz.order.shoppingcart.product.ProductPromoWorker.doPromotions(ProductPromoWorker.java:334)
> org.ofbiz.order.shoppingcart.product.ProductPromoWorker.doPromotions(ProductPromoWorker.java:293)
> org.ofbiz.order.shoppingcart.ShoppingCartItem.setQuantity(ShoppingCartItem.java:1061)
> org.ofbiz.order.shoppingcart.ShoppingCartItem.makeItem(ShoppingCartItem.java:502)
> org.ofbiz.order.shoppingcart.ShoppingCartItem.makeItem(ShoppingCartItem.java:334)
> org.ofbiz.order.shoppingcart.ShoppingCart.addOrIncreaseItem(ShoppingCart.java:590)
> org.ofbiz.order.shoppingcart.ShoppingCartHelper.addToCart(ShoppingCartHelper.java:245)
> org.ofbiz.order.shoppingcart.ShoppingCartEvents.addToCart(ShoppingCartEvents.java:638)
> sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
> sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:57)
> sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
> java.lang.reflect.Method.invoke(Method.java:606)
> org.ofbiz.webapp.event.JavaEventHandler.invoke(JavaEventHandler.java:93)
> org.ofbiz.webapp.event.JavaEventHandler.invoke(JavaEventHandler.java:79)
> org.ofbiz.webapp.control.RequestHandler.runEvent(RequestHandler.java:749)
> org.ofbiz.webapp.control.RequestHandler.doRequest(RequestHandler.java:469)
> org.ofbiz.webapp.control.ControlServlet.doGet(ControlServlet.java:219)
> org.ofbiz.webapp.control.ControlServlet.doPost(ControlServlet.java:91)
> javax.servlet.http.HttpServlet.service(HttpServlet.java:641)
> javax.servlet.http.HttpServlet.service(HttpServlet.java:722)
> org.apache.catalina.core.ApplicationFilterChain.internalDoFilter(ApplicationFilterChain.java:305)
> org.apache.catalina.core.ApplicationFilterChain.doFilter(ApplicationFilterChain.java:210)
> org.ofbiz.webapp.control.ContextFilter.doFilter(ContextFilter.java:314)
> org.apache.catalina.core.ApplicationFilterChain.internalDoFilter(ApplicationFilterChain.java:243)
> org.apache.catalina.core.ApplicationFilterChain.doFilter(ApplicationFilterChain.java:210)
> org.apache.catalina.core.StandardWrapperValve.invoke(StandardWrapperValve.java:222)
> org.apache.catalina.core.StandardContextValve.invoke(StandardContextValve.java:123)
> org.apache.catalina.authenticator.AuthenticatorBase.invoke(AuthenticatorBase.java:502)
> org.apache.catalina.core.StandardHostValve.invoke(StandardHostValve.java:171)
> org.apache.catalina.valves.ErrorReportValve.invoke(ErrorReportValve.java:100)
> org.apache.catalina.core.StandardEngineValve.invoke(StandardEngineValve.java:118)
> org.apache.catalina.valves.AccessLogValve.invoke(AccessLogValve.java:953)
> org.apache.catalina.connector.CoyoteAdapter.service(CoyoteAdapter.java:409)
> org.apache.coyote.http11.AbstractHttp11Processor.process(AbstractHttp11Processor.java:1044)
> org.apache.coyote.AbstractProtocol$AbstractConnectionHandler.process(AbstractProtocol.java:607)
> org.apache.tomcat.util.net.JIoEndpoint$SocketProcessor.run(JIoEndpoint.java:315)
> java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1145)
> java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:615)
> java.lang.Thread.run(Thread.java:744)
> --------------------------------------------------------------------------------
>
>
> -Adrian
>
>
>
> Quoting [hidden email]:
>
>> Author: jacopoc
>> Date: Mon Dec 30 15:53:07 2013
>> New Revision: 1554265
>>
>> URL: http://svn.apache.org/r1554265
>> Log:
>> This is a refactoring of the product promotion engine in order to overcome some limitations that prevented it to select and apply the best set of promotions under special conditions.
>>
>> Example: Consider two promotions:
>> * consider two products: Product A, with price $20, and Product B, with price $40
>> * Promotion 1: 20% discount on all the products in a category containing Product A and Product B
>> * Promotion 2: 40% discount on Product A
>>
>> When Product A and Product B are both in the cart:
>> * Expected behavior: on Product A the Promotion 2 should be applied i.e. 40% discount, and on Product B Promotion 1 should be applied i.e. 20% discount.
>> ** Summary
>> Product Price Discount Subtotal
>> A $20 $8 (40% discount) $12
>> B $40 $8 (20% discount) $32
>> Total Adjustment: $16
>>
>> * OFBiz behavior (before this fix): Promotion 1 is applied to Product A and Product B; this happens because the total discount of Promotion 1 is greater than the total discount of Promotion 2 and OFBiz applies promotions sorted by discount (desc)
>> ** Summary
>> Product Price Discount Subtotal
>> A $20 $4 (20% discount) $16
>> B $40 $8 (20% discount) $32
>> Total Adjustment: $12
>>
>> The new solution fixes this issue and similar ones.
>>
>> Here are some details about the new algorithm.
>>
>> Overview of the flow:
>> 1) run the promotions one by one in a test run
>> 2) collect the ProductPromoUse information
>> 3) sort them by weight (i.e. the ratio between the discount and the value of the products discounted)
>> 4) execute the ProductPromoUse in the given order
>>
>> In order to understand this solution, and specifically the changes to ProductPromoWorker.java, there is an important concept to consider:
>> one Promotion can generate more than one ProductPromoUseInfo objects.
>> For example if I have 2 units of WG-1111 in the cart (in one cart item) and I have the promotion “20% discount on WG-1111 and GZ-1000” then the system will create TWO ProductPromoUseInfo objects both associated to the same promotion one for each of the 2 units discounted.
>> Similarly if I had two lines: 2 units of WG-1111 and 1 unit of GZ-1000 I would get 3 ProductPromoUseInfo objects 2 objects for WG-1111 and 1 object for GZ-1000
>>
>> We can sort these ProductPromoUseInfo objects based on their weight (i.e. the ratio between the discount and the value of the products discounted) in desc order
>> and now we have a sorted list of ProductPromoUseInfo objects ready to be executed
>> However we only want to execute each of them once and for this reason we set (in memory, not in the DB) the useLimitPerOrder to 1 in the first ProductPromoUseInfo of a given promotion and then to 2 if the same ProductPromoUseInfo is associated to the same promotion etc...
>> in this way the end result is that the system will generate, as we desire, ONE ProductPromoUseInfo only for each of the ProductPromoUseInfo in the list.
>>
>> Here is an example:
>> we have 2 promotions:
>> PROMO A
>> PROMO B
>>
>> After test run:
>>
>> ProductPromoUseInfo - PROMO A - #1 - weight 0.3
>> ProductPromoUseInfo - PROMO A - #2 - weight 0.3
>> ProductPromoUseInfo - PROMO B - #1 - weight 0.4
>>
>> After sorting:
>>
>> ProductPromoUseInfo - PROMO B - #1 - weight 0.4
>> ProductPromoUseInfo - PROMO A - #1 - weight 0.3
>> ProductPromoUseInfo - PROMO A - #2 - weight 0.3
>>
>> Based on this we create a list (sortedExplodedProductPromoList) of ProductPromo:
>>
>> PROMO B - with useLimitPerOrder=1
>> PROMO A - with useLimitPerOrder=1
>> PROMO A - with useLimitPerOrder=2
>>
>> When we apply these to the cart we get the following results:
>>
>> PROMO B - with useLimitPerOrder=1 APPLIED
>> PROMO A - with useLimitPerOrder=1 APPLIED
>> PROMO A - with useLimitPerOrder=2 NOT APPLIED (because PROMO B used the item)
>>
>>
>> Modified:
>>    ofbiz/trunk/applications/order/src/org/ofbiz/order/shoppingcart/ShoppingCart.java
>>    ofbiz/trunk/applications/order/src/org/ofbiz/order/shoppingcart/ShoppingCartServices.java
>>    ofbiz/trunk/applications/order/src/org/ofbiz/order/shoppingcart/product/ProductPromoWorker.java
>>
>> Modified: ofbiz/trunk/applications/order/src/org/ofbiz/order/shoppingcart/ShoppingCart.java
>> URL: http://svn.apache.org/viewvc/ofbiz/trunk/applications/order/src/org/ofbiz/order/shoppingcart/ShoppingCart.java?rev=1554265&r1=1554264&r2=1554265&view=diff
>> ==============================================================================
>> --- ofbiz/trunk/applications/order/src/org/ofbiz/order/shoppingcart/ShoppingCart.java (original)
>> +++ ofbiz/trunk/applications/order/src/org/ofbiz/order/shoppingcart/ShoppingCart.java Mon Dec 30 15:53:07 2013
>> @@ -2714,7 +2714,7 @@ public class ShoppingCart implements Ite
>>             }
>>             itemsTotal = itemsTotal.add(cartItem.getItemSubTotal());
>>         }
>> -        return itemsTotal;
>> +        return itemsTotal.add(this.getOrderOtherAdjustmentTotal());
>>     }
>>
>>     /**
>> @@ -3142,12 +3142,12 @@ public class ShoppingCart implements Ite
>>         return new HashMap<GenericPK, String>(this.desiredAlternateGiftByAction);
>>     }
>>
>> -    public void addProductPromoUse(String productPromoId, String productPromoCodeId, BigDecimal totalDiscountAmount, BigDecimal quantityLeftInActions) {
>> +    public void addProductPromoUse(String productPromoId, String productPromoCodeId, BigDecimal totalDiscountAmount, BigDecimal quantityLeftInActions, Map<ShoppingCartItem,BigDecimal> usageInfoMap) {
>>         if (UtilValidate.isNotEmpty(productPromoCodeId) && !this.productPromoCodes.contains(productPromoCodeId)) {
>>             throw new IllegalStateException("Cannot add a use to a promo code use for a code that has not been entered.");
>>         }
>>         if (Debug.verboseOn()) Debug.logVerbose("Used promotion [" + productPromoId + "] with code [" + productPromoCodeId + "] for total discount [" + totalDiscountAmount + "] and quantity left in actions [" + quantityLeftInActions + "]", module);
>> -        this.productPromoUseInfoList.add(new ProductPromoUseInfo(productPromoId, productPromoCodeId, totalDiscountAmount, quantityLeftInActions));
>> +        this.productPromoUseInfoList.add(new ProductPromoUseInfo(productPromoId, productPromoCodeId, totalDiscountAmount, quantityLeftInActions, usageInfoMap));
>>     }
>>
>>     public void removeProductPromoUse(String productPromoId) {
>> @@ -4385,23 +4385,43 @@ public class ShoppingCart implements Ite
>>         }
>>     }
>>
>> -    public static class ProductPromoUseInfo implements Serializable {
>> +    public static class ProductPromoUseInfo implements Serializable, Comparable<ProductPromoUseInfo> {
>>         public String productPromoId = null;
>>         public String productPromoCodeId = null;
>>         public BigDecimal totalDiscountAmount = BigDecimal.ZERO;
>>         public BigDecimal quantityLeftInActions = BigDecimal.ZERO;
>> +        private Map<ShoppingCartItem,BigDecimal> usageInfoMap = null;
>>
>> -        public ProductPromoUseInfo(String productPromoId, String productPromoCodeId, BigDecimal totalDiscountAmount, BigDecimal quantityLeftInActions) {
>> +        public ProductPromoUseInfo(String productPromoId, String productPromoCodeId, BigDecimal totalDiscountAmount, BigDecimal quantityLeftInActions, Map<ShoppingCartItem,BigDecimal> usageInfoMap) {
>>             this.productPromoId = productPromoId;
>>             this.productPromoCodeId = productPromoCodeId;
>>             this.totalDiscountAmount = totalDiscountAmount;
>>             this.quantityLeftInActions = quantityLeftInActions;
>> +            this.usageInfoMap = usageInfoMap;
>>         }
>>
>>         public String getProductPromoId() { return this.productPromoId; }
>>         public String getProductPromoCodeId() { return this.productPromoCodeId; }
>>         public BigDecimal getTotalDiscountAmount() { return this.totalDiscountAmount; }
>>         public BigDecimal getQuantityLeftInActions() { return this.quantityLeftInActions; }
>> +        public Map<ShoppingCartItem,BigDecimal> getUsageInfoMap() { return this.usageInfoMap; }
>> +        public BigDecimal getUsageWeight() {
>> +            Iterator<ShoppingCartItem> lineItems = this.usageInfoMap.keySet().iterator();
>> +            BigDecimal totalAmount = BigDecimal.ZERO;
>> +            while (lineItems.hasNext()) {
>> +                ShoppingCartItem lineItem = lineItems.next();
>> +                totalAmount = totalAmount.add(lineItem.getBasePrice().multiply(usageInfoMap.get(lineItem)));
>> +            }
>> +            if (totalAmount.compareTo(BigDecimal.ZERO) == 0) {
>> +                return BigDecimal.ZERO;
>> +            } else {
>> +                return getTotalDiscountAmount().negate().divide(totalAmount);
>> +            }
>> +        }
>> +
>> +        public int compareTo(ProductPromoUseInfo other) {
>> +            return other.getUsageWeight().compareTo(getUsageWeight());
>> +        }
>>     }
>>
>>     public static class CartShipInfo implements Serializable {
>>
>> Modified: ofbiz/trunk/applications/order/src/org/ofbiz/order/shoppingcart/ShoppingCartServices.java
>> URL: http://svn.apache.org/viewvc/ofbiz/trunk/applications/order/src/org/ofbiz/order/shoppingcart/ShoppingCartServices.java?rev=1554265&r1=1554264&r2=1554265&view=diff
>> ==============================================================================
>> --- ofbiz/trunk/applications/order/src/org/ofbiz/order/shoppingcart/ShoppingCartServices.java (original)
>> +++ ofbiz/trunk/applications/order/src/org/ofbiz/order/shoppingcart/ShoppingCartServices.java Mon Dec 30 15:53:07 2013
>> @@ -21,6 +21,7 @@ package org.ofbiz.order.shoppingcart;
>> import java.math.BigDecimal;
>> import java.math.MathContext;
>> import java.sql.Timestamp;
>> +import java.util.HashMap;
>> import java.util.Iterator;
>> import java.util.List;
>> import java.util.Locale;
>> @@ -629,7 +630,7 @@ public class ShoppingCartServices {
>>                 cart.addProductPromoCode(productPromoCode, dispatcher);
>>             }
>>             for (GenericValue productPromoUse: orh.getProductPromoUse()) {
>> -                cart.addProductPromoUse(productPromoUse.getString("productPromoId"), productPromoUse.getString("productPromoCodeId"), productPromoUse.getBigDecimal("totalDiscountAmount"), productPromoUse.getBigDecimal("quantityLeftInActions"));
>> +                cart.addProductPromoUse(productPromoUse.getString("productPromoId"), productPromoUse.getString("productPromoCodeId"), productPromoUse.getBigDecimal("totalDiscountAmount"), productPromoUse.getBigDecimal("quantityLeftInActions"), new HashMap<ShoppingCartItem, BigDecimal>());
>>             }
>>         }
>>
>>
>> Modified: ofbiz/trunk/applications/order/src/org/ofbiz/order/shoppingcart/product/ProductPromoWorker.java
>> URL: http://svn.apache.org/viewvc/ofbiz/trunk/applications/order/src/org/ofbiz/order/shoppingcart/product/ProductPromoWorker.java?rev=1554265&r1=1554264&r2=1554265&view=diff
>> ==============================================================================
>> --- ofbiz/trunk/applications/order/src/org/ofbiz/order/shoppingcart/product/ProductPromoWorker.java (original)
>> +++ ofbiz/trunk/applications/order/src/org/ofbiz/order/shoppingcart/product/ProductPromoWorker.java Mon Dec 30 15:53:07 2013
>> @@ -22,6 +22,8 @@ import java.math.BigDecimal;
>> import java.math.MathContext;
>> import java.sql.Timestamp;
>> import java.util.ArrayList;
>> +import java.util.Collections;
>> +import java.util.HashMap;
>> import java.util.Iterator;
>> import java.util.List;
>> import java.util.Locale;
>> @@ -51,6 +53,7 @@ import org.ofbiz.entity.condition.Entity
>> import org.ofbiz.entity.util.EntityUtil;
>> import org.ofbiz.order.shoppingcart.CartItemModifyException;
>> import org.ofbiz.order.shoppingcart.ShoppingCart;
>> +import org.ofbiz.order.shoppingcart.ShoppingCart.ProductPromoUseInfo;
>> import org.ofbiz.order.shoppingcart.ShoppingCartEvents;
>> import org.ofbiz.order.shoppingcart.ShoppingCartItem;
>> import org.ofbiz.product.product.ProductContentWrapper;
>> @@ -318,44 +321,62 @@ public class ProductPromoWorker {
>>             // do a calculate only run through the promotions, then order by descending totalDiscountAmount for each promotion
>>             // NOTE: on this run, with isolatedTestRun passed as false it should not apply any adjustments
>>             //  or track which cart items are used for which promotions, but it will track ProductPromoUseInfo and
>> -            //  useLimits; we are basicly just trying to run each promo "independently" to see how much each is worth
>> +            //  useLimits; we are basically just trying to run each promo "independently" to see how much each is worth
>>             runProductPromos(productPromoList, cart, delegator, dispatcher, nowTimestamp, true);
>>
>> -            // NOTE: after that first pass we could remove any that have a 0 totalDiscountAmount from the run list, but we won't because by the time they are run the cart may have changed enough to get them to go; also, certain actions like free shipping should always be run even though we won't know what the totalDiscountAmount is at the time the promotion is run
>> -            // each ProductPromoUseInfo on the shopping cart will contain it's total value, so add up all totals for each promoId and put them in a List of Maps
>> -            // create a List of Maps with productPromo and totalDiscountAmount, use the Map sorter to sort them descending by totalDiscountAmount
>> -
>> -            // before sorting split into two lists and sort each list; one list for promos that have a order total condition, and the other list for all promos that don't; then we'll always run the ones that have no condition on the order total first
>> -            List<Map<Object, Object>> productPromoDiscountMapList = FastList.newInstance();
>> -            List<Map<Object, Object>> productPromoDiscountMapListOrderTotal = FastList.newInstance();
>> +            // NOTE: we can easily recognize the promos for the order total: they are the ones with usage set to 0
>> +            Iterator<ProductPromoUseInfo> promoUses = cart.getProductPromoUseInfoIter();
>> +            List<ProductPromoUseInfo> sortedPromoUses = new ArrayList<ProductPromoUseInfo>();
>> +            while (promoUses.hasNext()) {
>> +                ProductPromoUseInfo promoUse = promoUses.next();
>> +                sortedPromoUses.add(promoUse);
>> +            }
>> +            Collections.sort(sortedPromoUses);
>> +            List<GenericValue> sortedExplodedProductPromoList = new ArrayList<GenericValue>(sortedPromoUses.size());
>> +            Map<String, Long> usesPerPromo = new HashMap<String, Long>();
>> +            int indexOfFirstOrderTotalPromo = -1;
>> +            for (ProductPromoUseInfo promoUse: sortedPromoUses) {
>> +                GenericValue productPromo = delegator.findOne("ProductPromo", UtilMisc.toMap("productPromoId", promoUse.getProductPromoId()), true);
>> +                GenericValue newProductPromo = (GenericValue)productPromo.clone();
>> +                if (!usesPerPromo.containsKey(promoUse.getProductPromoId())) {
>> +                    usesPerPromo.put(promoUse.getProductPromoId(), 0l);
>> +                }
>> +                long uses = usesPerPromo.get(promoUse.getProductPromoId());
>> +                uses = uses + 1;
>> +                long useLimitPerOrder = (newProductPromo.get("useLimitPerOrder") != null? newProductPromo.getLong("useLimitPerOrder"): -1);
>> +                if (useLimitPerOrder == -1 || uses < useLimitPerOrder) {
>> +                    newProductPromo.set("useLimitPerOrder", uses);
>> +                }
>> +                usesPerPromo.put(promoUse.getProductPromoId(), uses);
>> +                sortedExplodedProductPromoList.add(newProductPromo);
>> +                if (indexOfFirstOrderTotalPromo == -1 && BigDecimal.ZERO.equals(promoUse.getUsageWeight())) {
>> +                    indexOfFirstOrderTotalPromo = sortedExplodedProductPromoList.size() - 1;
>> +                }
>> +            }
>> +            if (indexOfFirstOrderTotalPromo == -1) {
>> +                indexOfFirstOrderTotalPromo = sortedExplodedProductPromoList.size() - 1;
>> +            }
>> +
>>             for (GenericValue productPromo : productPromoList) {
>>                 Map<Object, Object> productPromoDiscountMap = UtilGenerics.checkMap(UtilMisc.toMap("productPromo", productPromo, "totalDiscountAmount", cart.getProductPromoUseTotalDiscount(productPromo.getString("productPromoId"))));
>>                 if (hasOrderTotalCondition(productPromo, delegator)) {
>> -                    productPromoDiscountMapListOrderTotal.add(productPromoDiscountMap);
>> +                    if (!usesPerPromo.containsKey(productPromo.getString("productPromoId"))) {
>> +                        sortedExplodedProductPromoList.add(productPromo);
>> +                    }
>>                 } else {
>> -                    productPromoDiscountMapList.add(productPromoDiscountMap);
>> +                    if (!usesPerPromo.containsKey(productPromo.getString("productPromoId"))) {
>> +                        if (indexOfFirstOrderTotalPromo != -1) {
>> +                            sortedExplodedProductPromoList.add(indexOfFirstOrderTotalPromo, productPromo);
>> +                        } else {
>> +                            sortedExplodedProductPromoList.add(0, productPromo);
>> +                        }
>> +                    }
>>                 }
>>             }
>>
>> -
>> -            // sort the Map List, do it ascending because the discount amounts will be negative, so the lowest number is really the highest discount
>> -            productPromoDiscountMapList = UtilMisc.sortMaps(productPromoDiscountMapList, UtilMisc.toList("+totalDiscountAmount"));
>> -            productPromoDiscountMapListOrderTotal = UtilMisc.sortMaps(productPromoDiscountMapListOrderTotal, UtilMisc.toList("+totalDiscountAmount"));
>> -
>> -            productPromoDiscountMapList.addAll(productPromoDiscountMapListOrderTotal);
>> -
>> -            List<GenericValue> sortedProductPromoList = new ArrayList<GenericValue>(productPromoDiscountMapList.size());
>> -            Iterator<Map<Object, Object>> productPromoDiscountMapIter = productPromoDiscountMapList.iterator();
>> -            while (productPromoDiscountMapIter.hasNext()) {
>> -                Map<Object, Object> productPromoDiscountMap = UtilGenerics.checkMap(productPromoDiscountMapIter.next());
>> -                GenericValue productPromo = (GenericValue) productPromoDiscountMap.get("productPromo");
>> -                sortedProductPromoList.add(productPromo);
>> -                if (Debug.verboseOn()) Debug.logVerbose("Sorted Promo [" + productPromo.getString("productPromoId") + "] with total discount: " + productPromoDiscountMap.get("totalDiscountAmount"), module);
>> -            }
>> -
>>             // okay, all ready, do the real run, clearing the temporary result first...
>>             cart.clearAllPromotionInformation();
>> -            runProductPromos(sortedProductPromoList, cart, delegator, dispatcher, nowTimestamp, false);
>> +            runProductPromos(sortedExplodedProductPromoList, cart, delegator, dispatcher, nowTimestamp, false);
>>         } catch (NumberFormatException e) {
>>             Debug.logError(e, "Number not formatted correctly in promotion rules, not completed...", module);
>>         } catch (GenericEntityException e) {
>> @@ -436,7 +457,7 @@ public class ProductPromoWorker {
>>                                     GenericValue productPromoCode = productPromoCodeIter.next();
>>                                     String productPromoCodeId = productPromoCode.getString("productPromoCodeId");
>>                                     Long codeUseLimit = getProductPromoCodeUseLimit(productPromoCode, partyId, delegator);
>> -                                    if (runProductPromoRules(cart, cartChanged, useLimit, true, productPromoCodeId, codeUseLimit, maxUseLimit, productPromo, productPromoRules, dispatcher, delegator, nowTimestamp)) {
>> +                                    if (runProductPromoRules(cart, useLimit, true, productPromoCodeId, codeUseLimit, maxUseLimit, productPromo, productPromoRules, dispatcher, delegator, nowTimestamp)) {
>>                                         cartChanged = true;
>>                                     }
>>
>> @@ -448,7 +469,7 @@ public class ProductPromoWorker {
>>                             }
>>                         } else {
>>                             try {
>> -                                if (runProductPromoRules(cart, cartChanged, useLimit, false, null, null, maxUseLimit, productPromo, productPromoRules, dispatcher, delegator, nowTimestamp)) {
>> +                                if (runProductPromoRules(cart, useLimit, false, null, null, maxUseLimit, productPromo, productPromoRules, dispatcher, delegator, nowTimestamp)) {
>>                                     cartChanged = true;
>>                                 }
>>                             } catch (RuntimeException e) {
>> @@ -735,8 +756,10 @@ public class ProductPromoWorker {
>>         return promoDescBuf.toString();
>>     }
>>
>> -    protected static boolean runProductPromoRules(ShoppingCart cart, boolean cartChanged, Long useLimit, boolean requireCode, String productPromoCodeId, Long codeUseLimit, long maxUseLimit,
>> +    protected static boolean runProductPromoRules(ShoppingCart cart, Long useLimit, boolean requireCode, String productPromoCodeId, Long codeUseLimit, long maxUseLimit,
>>         GenericValue productPromo, List<GenericValue> productPromoRules, LocalDispatcher dispatcher, Delegator delegator, Timestamp nowTimestamp) throws GenericEntityException, UseLimitException {
>> +        boolean cartChanged = false;
>> +        Map<ShoppingCartItem,BigDecimal> usageInfoMap = prepareProductUsageInfoMap(cart);
>>         String productPromoId = productPromo.getString("productPromoId");
>>         while ((useLimit == null || useLimit.longValue() > cart.getProductPromoUseCount(productPromoId)) &&
>>                 (!requireCode || UtilValidate.isNotEmpty(productPromoCodeId)) &&
>> @@ -755,17 +778,17 @@ public class ProductPromoWorker {
>>                 // loop through conditions for rule, if any false, set allConditionsTrue to false
>>                 List<GenericValue> productPromoConds = delegator.findByAnd("ProductPromoCond", UtilMisc.toMap("productPromoId", productPromo.get("productPromoId")), UtilMisc.toList("productPromoCondSeqId"), true);
>>                 productPromoConds = EntityUtil.filterByAnd(productPromoConds, UtilMisc.toMap("productPromoRuleId", productPromoRule.get("productPromoRuleId")));
>> -                // using the other method to consolodate cache entries because the same cache is used elsewhere: List productPromoConds = productPromoRule.getRelated("ProductPromoCond", null, UtilMisc.toList("productPromoCondSeqId"), true);
>> +                // using the other method to consolidate cache entries because the same cache is used elsewhere: List productPromoConds = productPromoRule.getRelated("ProductPromoCond", null, UtilMisc.toList("productPromoCondSeqId"), true);
>>                 if (Debug.verboseOn()) Debug.logVerbose("Checking " + productPromoConds.size() + " conditions for rule " + productPromoRule, module);
>>
>>                 Iterator<GenericValue> productPromoCondIter = UtilMisc.toIterator(productPromoConds);
>>                 while (productPromoCondIter != null && productPromoCondIter.hasNext()) {
>>                     GenericValue productPromoCond = productPromoCondIter.next();
>>
>> -                    boolean condResult = checkCondition(productPromoCond, cart, delegator, dispatcher, nowTimestamp);
>> +                    boolean conditionSatisfied = checkCondition(productPromoCond, cart, delegator, dispatcher, nowTimestamp);
>>
>>                     // any false condition will cause it to NOT perform the action
>> -                    if (condResult == false) {
>> +                    if (!conditionSatisfied) {
>>                         performActions = false;
>>                         break;
>>                     }
>> @@ -797,13 +820,16 @@ public class ProductPromoWorker {
>>             }
>>
>>             if (promoUsed) {
>> -                cart.addProductPromoUse(productPromo.getString("productPromoId"), productPromoCodeId, totalDiscountAmount, quantityLeftInActions);
>> +                // Get product use information from the cart
>> +                Map<ShoppingCartItem,BigDecimal> newUsageInfoMap = prepareProductUsageInfoMap(cart);
>> +                Map<ShoppingCartItem,BigDecimal> deltaUsageInfoMap = prepareDeltaProductUsageInfoMap(usageInfoMap, newUsageInfoMap);
>> +                usageInfoMap = newUsageInfoMap;
>> +                cart.addProductPromoUse(productPromo.getString("productPromoId"), productPromoCodeId, totalDiscountAmount, quantityLeftInActions, deltaUsageInfoMap);
>>             } else {
>>                 // the promotion was not used, don't try again until we finish a full pass and come back to see the promo conditions are now satisfied based on changes to the cart
>>                 break;
>>             }
>>
>> -
>>             if (cart.getProductPromoUseCount(productPromoId) > maxUseLimit) {
>>                 throw new UseLimitException("ERROR: While calculating promotions the promotion [" + productPromoId + "] action was applied more than " + maxUseLimit + " times, so the calculation has been ended. This should generally never happen unless you have bad rule definitions.");
>>             }
>> @@ -812,6 +838,34 @@ public class ProductPromoWorker {
>>         return cartChanged;
>>     }
>>
>> +    private static Map<ShoppingCartItem,BigDecimal> prepareProductUsageInfoMap(ShoppingCart cart) {
>> +        Map<ShoppingCartItem,BigDecimal> usageInfoMap = new HashMap<ShoppingCartItem, BigDecimal>();
>> +        List<ShoppingCartItem> lineOrderedByBasePriceList = cart.getLineListOrderedByBasePrice(false);
>> +        for (ShoppingCartItem cartItem : lineOrderedByBasePriceList) {
>> +            BigDecimal used = cartItem.getPromoQuantityUsed();
>> +            if (used.compareTo(BigDecimal.ZERO) != 0) {
>> +                usageInfoMap.put(cartItem, used);
>> +            }
>> +        }
>> +        return usageInfoMap;
>> +    }
>> +
>> +    private static Map<ShoppingCartItem,BigDecimal> prepareDeltaProductUsageInfoMap(Map<ShoppingCartItem,BigDecimal> oldMap, Map<ShoppingCartItem,BigDecimal> newMap) {
>> +        Map<ShoppingCartItem,BigDecimal> deltaUsageInfoMap = new HashMap<ShoppingCartItem, BigDecimal>(newMap);
>> +        Iterator<ShoppingCartItem> cartLines = oldMap.keySet().iterator();
>> +        while (cartLines.hasNext()) {
>> +            ShoppingCartItem cartLine = cartLines.next();
>> +            BigDecimal oldUsed = oldMap.get(cartLine);
>> +            BigDecimal newUsed = newMap.get(cartLine);
>> +            if (newUsed.compareTo(oldUsed) > 0) {
>> +                deltaUsageInfoMap.put(cartLine, newUsed.add(oldUsed.negate()));
>> +            } else {
>> +                deltaUsageInfoMap.remove(cartLine);
>> +            }
>> +        }
>> +        return deltaUsageInfoMap;
>> +    }
>> +
>>     protected static boolean checkCondition(GenericValue productPromoCond, ShoppingCart cart, Delegator delegator, LocalDispatcher dispatcher, Timestamp nowTimestamp) throws GenericEntityException {
>>         String condValue = productPromoCond.getString("condValue");
>>         String otherValue = productPromoCond.getString("otherValue");
>> @@ -1772,8 +1826,8 @@ public class ProductPromoWorker {
>>             actionResultInfo.ranAction = false;
>>         }
>>
>> +        // in action, if doesn't have enough quantity to use the promo at all, remove candidate promo uses and increment promoQuantityUsed; this should go for all actions, if any action runs we confirm
>>         if (actionResultInfo.ranAction) {
>> -            // in action, if doesn't have enough quantity to use the promo at all, remove candidate promo uses and increment promoQuantityUsed; this should go for all actions, if any action runs we confirm
>>             cart.confirmPromoRuleUse(productPromoAction.getString("productPromoId"), productPromoAction.getString("productPromoRuleId"));
>>         } else {
>>             cart.resetPromoRuleUse(productPromoAction.getString("productPromoId"), productPromoAction.getString("productPromoRuleId"));
>>
>>
>>
>
>
>

Reply | Threaded
Open this post in threaded view
|

Re: svn commit: r1554265 - in /ofbiz/trunk/applications/order/src/org/ofbiz/order/shoppingcart: ShoppingCart.java ShoppingCartServices.java product/ProductPromoWorker.java

Jacopo Cappellato-4
Fixed at rev. 1558145
Thanks for the report.

Jacopo

On Jan 14, 2014, at 6:36 PM, Jacopo Cappellato <[hidden email]> wrote:

> Thanks Adrian,
>
> I am looking into this.
>
> Jacopo
>
> On Jan 14, 2014, at 6:24 PM, [hidden email] wrote:
>
>> This commit breaks promotions.
>>
>> 1. Create a new sales order, customer DemoCustomer.
>> 2. Add GZ-8544, qty 1 to the order.
>> 3. Promo items do not appear in order.
>> 4. Log shows an exception being thrown:
>>
>> 2014-01-14 12:15:56,030 (http-bio-0.0.0.0-8443-exec-1) [ ProductPromoWorker.java:385:ERROR]
>> ---- runtime exception report --------------------------------------------------
>> Error running promotions, will ignore: java.lang.ArithmeticException: Non-terminating decimal expansion; no exact representable decimal result.
>> Exception: java.lang.ArithmeticException
>> Message: Non-terminating decimal expansion; no exact representable decimal result.
>> ---- stack trace ---------------------------------------------------------------
>> java.lang.ArithmeticException: Non-terminating decimal expansion; no exact representable decimal result.
>> java.math.BigDecimal.divide(BigDecimal.java:1616)
>> org.ofbiz.order.shoppingcart.ShoppingCart$ProductPromoUseInfo.getUsageWeight(ShoppingCart.java:4418)
>> org.ofbiz.order.shoppingcart.ShoppingCart$ProductPromoUseInfo.compareTo(ShoppingCart.java:4423)
>> org.ofbiz.order.shoppingcart.ShoppingCart$ProductPromoUseInfo.compareTo(ShoppingCart.java:4388)
>> java.util.ComparableTimSort.countRunAndMakeAscending(ComparableTimSort.java:290)
>> java.util.ComparableTimSort.sort(ComparableTimSort.java:157)
>> java.util.ComparableTimSort.sort(ComparableTimSort.java:146)
>> java.util.Arrays.sort(Arrays.java:472)
>> java.util.Collections.sort(Collections.java:155)
>> org.ofbiz.order.shoppingcart.product.ProductPromoWorker.doPromotions(ProductPromoWorker.java:334)
>> org.ofbiz.order.shoppingcart.product.ProductPromoWorker.doPromotions(ProductPromoWorker.java:293)
>> org.ofbiz.order.shoppingcart.ShoppingCartItem.setQuantity(ShoppingCartItem.java:1061)
>> org.ofbiz.order.shoppingcart.ShoppingCartItem.makeItem(ShoppingCartItem.java:502)
>> org.ofbiz.order.shoppingcart.ShoppingCartItem.makeItem(ShoppingCartItem.java:334)
>> org.ofbiz.order.shoppingcart.ShoppingCart.addOrIncreaseItem(ShoppingCart.java:590)
>> org.ofbiz.order.shoppingcart.ShoppingCartHelper.addToCart(ShoppingCartHelper.java:245)
>> org.ofbiz.order.shoppingcart.ShoppingCartEvents.addToCart(ShoppingCartEvents.java:638)
>> sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
>> sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:57)
>> sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
>> java.lang.reflect.Method.invoke(Method.java:606)
>> org.ofbiz.webapp.event.JavaEventHandler.invoke(JavaEventHandler.java:93)
>> org.ofbiz.webapp.event.JavaEventHandler.invoke(JavaEventHandler.java:79)
>> org.ofbiz.webapp.control.RequestHandler.runEvent(RequestHandler.java:749)
>> org.ofbiz.webapp.control.RequestHandler.doRequest(RequestHandler.java:469)
>> org.ofbiz.webapp.control.ControlServlet.doGet(ControlServlet.java:219)
>> org.ofbiz.webapp.control.ControlServlet.doPost(ControlServlet.java:91)
>> javax.servlet.http.HttpServlet.service(HttpServlet.java:641)
>> javax.servlet.http.HttpServlet.service(HttpServlet.java:722)
>> org.apache.catalina.core.ApplicationFilterChain.internalDoFilter(ApplicationFilterChain.java:305)
>> org.apache.catalina.core.ApplicationFilterChain.doFilter(ApplicationFilterChain.java:210)
>> org.ofbiz.webapp.control.ContextFilter.doFilter(ContextFilter.java:314)
>> org.apache.catalina.core.ApplicationFilterChain.internalDoFilter(ApplicationFilterChain.java:243)
>> org.apache.catalina.core.ApplicationFilterChain.doFilter(ApplicationFilterChain.java:210)
>> org.apache.catalina.core.StandardWrapperValve.invoke(StandardWrapperValve.java:222)
>> org.apache.catalina.core.StandardContextValve.invoke(StandardContextValve.java:123)
>> org.apache.catalina.authenticator.AuthenticatorBase.invoke(AuthenticatorBase.java:502)
>> org.apache.catalina.core.StandardHostValve.invoke(StandardHostValve.java:171)
>> org.apache.catalina.valves.ErrorReportValve.invoke(ErrorReportValve.java:100)
>> org.apache.catalina.core.StandardEngineValve.invoke(StandardEngineValve.java:118)
>> org.apache.catalina.valves.AccessLogValve.invoke(AccessLogValve.java:953)
>> org.apache.catalina.connector.CoyoteAdapter.service(CoyoteAdapter.java:409)
>> org.apache.coyote.http11.AbstractHttp11Processor.process(AbstractHttp11Processor.java:1044)
>> org.apache.coyote.AbstractProtocol$AbstractConnectionHandler.process(AbstractProtocol.java:607)
>> org.apache.tomcat.util.net.JIoEndpoint$SocketProcessor.run(JIoEndpoint.java:315)
>> java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1145)
>> java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:615)
>> java.lang.Thread.run(Thread.java:744)
>> --------------------------------------------------------------------------------
>>
>>
>> -Adrian
>>
>>
>>
>> Quoting [hidden email]:
>>
>>> Author: jacopoc
>>> Date: Mon Dec 30 15:53:07 2013
>>> New Revision: 1554265
>>>
>>> URL: http://svn.apache.org/r1554265
>>> Log:
>>> This is a refactoring of the product promotion engine in order to overcome some limitations that prevented it to select and apply the best set of promotions under special conditions.
>>>
>>> Example: Consider two promotions:
>>> * consider two products: Product A, with price $20, and Product B, with price $40
>>> * Promotion 1: 20% discount on all the products in a category containing Product A and Product B
>>> * Promotion 2: 40% discount on Product A
>>>
>>> When Product A and Product B are both in the cart:
>>> * Expected behavior: on Product A the Promotion 2 should be applied i.e. 40% discount, and on Product B Promotion 1 should be applied i.e. 20% discount.
>>> ** Summary
>>> Product Price Discount Subtotal
>>> A $20 $8 (40% discount) $12
>>> B $40 $8 (20% discount) $32
>>> Total Adjustment: $16
>>>
>>> * OFBiz behavior (before this fix): Promotion 1 is applied to Product A and Product B; this happens because the total discount of Promotion 1 is greater than the total discount of Promotion 2 and OFBiz applies promotions sorted by discount (desc)
>>> ** Summary
>>> Product Price Discount Subtotal
>>> A $20 $4 (20% discount) $16
>>> B $40 $8 (20% discount) $32
>>> Total Adjustment: $12
>>>
>>> The new solution fixes this issue and similar ones.
>>>
>>> Here are some details about the new algorithm.
>>>
>>> Overview of the flow:
>>> 1) run the promotions one by one in a test run
>>> 2) collect the ProductPromoUse information
>>> 3) sort them by weight (i.e. the ratio between the discount and the value of the products discounted)
>>> 4) execute the ProductPromoUse in the given order
>>>
>>> In order to understand this solution, and specifically the changes to ProductPromoWorker.java, there is an important concept to consider:
>>> one Promotion can generate more than one ProductPromoUseInfo objects.
>>> For example if I have 2 units of WG-1111 in the cart (in one cart item) and I have the promotion “20% discount on WG-1111 and GZ-1000” then the system will create TWO ProductPromoUseInfo objects both associated to the same promotion one for each of the 2 units discounted.
>>> Similarly if I had two lines: 2 units of WG-1111 and 1 unit of GZ-1000 I would get 3 ProductPromoUseInfo objects 2 objects for WG-1111 and 1 object for GZ-1000
>>>
>>> We can sort these ProductPromoUseInfo objects based on their weight (i.e. the ratio between the discount and the value of the products discounted) in desc order
>>> and now we have a sorted list of ProductPromoUseInfo objects ready to be executed
>>> However we only want to execute each of them once and for this reason we set (in memory, not in the DB) the useLimitPerOrder to 1 in the first ProductPromoUseInfo of a given promotion and then to 2 if the same ProductPromoUseInfo is associated to the same promotion etc...
>>> in this way the end result is that the system will generate, as we desire, ONE ProductPromoUseInfo only for each of the ProductPromoUseInfo in the list.
>>>
>>> Here is an example:
>>> we have 2 promotions:
>>> PROMO A
>>> PROMO B
>>>
>>> After test run:
>>>
>>> ProductPromoUseInfo - PROMO A - #1 - weight 0.3
>>> ProductPromoUseInfo - PROMO A - #2 - weight 0.3
>>> ProductPromoUseInfo - PROMO B - #1 - weight 0.4
>>>
>>> After sorting:
>>>
>>> ProductPromoUseInfo - PROMO B - #1 - weight 0.4
>>> ProductPromoUseInfo - PROMO A - #1 - weight 0.3
>>> ProductPromoUseInfo - PROMO A - #2 - weight 0.3
>>>
>>> Based on this we create a list (sortedExplodedProductPromoList) of ProductPromo:
>>>
>>> PROMO B - with useLimitPerOrder=1
>>> PROMO A - with useLimitPerOrder=1
>>> PROMO A - with useLimitPerOrder=2
>>>
>>> When we apply these to the cart we get the following results:
>>>
>>> PROMO B - with useLimitPerOrder=1 APPLIED
>>> PROMO A - with useLimitPerOrder=1 APPLIED
>>> PROMO A - with useLimitPerOrder=2 NOT APPLIED (because PROMO B used the item)
>>>
>>>
>>> Modified:
>>>   ofbiz/trunk/applications/order/src/org/ofbiz/order/shoppingcart/ShoppingCart.java
>>>   ofbiz/trunk/applications/order/src/org/ofbiz/order/shoppingcart/ShoppingCartServices.java
>>>   ofbiz/trunk/applications/order/src/org/ofbiz/order/shoppingcart/product/ProductPromoWorker.java
>>>
>>> Modified: ofbiz/trunk/applications/order/src/org/ofbiz/order/shoppingcart/ShoppingCart.java
>>> URL: http://svn.apache.org/viewvc/ofbiz/trunk/applications/order/src/org/ofbiz/order/shoppingcart/ShoppingCart.java?rev=1554265&r1=1554264&r2=1554265&view=diff
>>> ==============================================================================
>>> --- ofbiz/trunk/applications/order/src/org/ofbiz/order/shoppingcart/ShoppingCart.java (original)
>>> +++ ofbiz/trunk/applications/order/src/org/ofbiz/order/shoppingcart/ShoppingCart.java Mon Dec 30 15:53:07 2013
>>> @@ -2714,7 +2714,7 @@ public class ShoppingCart implements Ite
>>>            }
>>>            itemsTotal = itemsTotal.add(cartItem.getItemSubTotal());
>>>        }
>>> -        return itemsTotal;
>>> +        return itemsTotal.add(this.getOrderOtherAdjustmentTotal());
>>>    }
>>>
>>>    /**
>>> @@ -3142,12 +3142,12 @@ public class ShoppingCart implements Ite
>>>        return new HashMap<GenericPK, String>(this.desiredAlternateGiftByAction);
>>>    }
>>>
>>> -    public void addProductPromoUse(String productPromoId, String productPromoCodeId, BigDecimal totalDiscountAmount, BigDecimal quantityLeftInActions) {
>>> +    public void addProductPromoUse(String productPromoId, String productPromoCodeId, BigDecimal totalDiscountAmount, BigDecimal quantityLeftInActions, Map<ShoppingCartItem,BigDecimal> usageInfoMap) {
>>>        if (UtilValidate.isNotEmpty(productPromoCodeId) && !this.productPromoCodes.contains(productPromoCodeId)) {
>>>            throw new IllegalStateException("Cannot add a use to a promo code use for a code that has not been entered.");
>>>        }
>>>        if (Debug.verboseOn()) Debug.logVerbose("Used promotion [" + productPromoId + "] with code [" + productPromoCodeId + "] for total discount [" + totalDiscountAmount + "] and quantity left in actions [" + quantityLeftInActions + "]", module);
>>> -        this.productPromoUseInfoList.add(new ProductPromoUseInfo(productPromoId, productPromoCodeId, totalDiscountAmount, quantityLeftInActions));
>>> +        this.productPromoUseInfoList.add(new ProductPromoUseInfo(productPromoId, productPromoCodeId, totalDiscountAmount, quantityLeftInActions, usageInfoMap));
>>>    }
>>>
>>>    public void removeProductPromoUse(String productPromoId) {
>>> @@ -4385,23 +4385,43 @@ public class ShoppingCart implements Ite
>>>        }
>>>    }
>>>
>>> -    public static class ProductPromoUseInfo implements Serializable {
>>> +    public static class ProductPromoUseInfo implements Serializable, Comparable<ProductPromoUseInfo> {
>>>        public String productPromoId = null;
>>>        public String productPromoCodeId = null;
>>>        public BigDecimal totalDiscountAmount = BigDecimal.ZERO;
>>>        public BigDecimal quantityLeftInActions = BigDecimal.ZERO;
>>> +        private Map<ShoppingCartItem,BigDecimal> usageInfoMap = null;
>>>
>>> -        public ProductPromoUseInfo(String productPromoId, String productPromoCodeId, BigDecimal totalDiscountAmount, BigDecimal quantityLeftInActions) {
>>> +        public ProductPromoUseInfo(String productPromoId, String productPromoCodeId, BigDecimal totalDiscountAmount, BigDecimal quantityLeftInActions, Map<ShoppingCartItem,BigDecimal> usageInfoMap) {
>>>            this.productPromoId = productPromoId;
>>>            this.productPromoCodeId = productPromoCodeId;
>>>            this.totalDiscountAmount = totalDiscountAmount;
>>>            this.quantityLeftInActions = quantityLeftInActions;
>>> +            this.usageInfoMap = usageInfoMap;
>>>        }
>>>
>>>        public String getProductPromoId() { return this.productPromoId; }
>>>        public String getProductPromoCodeId() { return this.productPromoCodeId; }
>>>        public BigDecimal getTotalDiscountAmount() { return this.totalDiscountAmount; }
>>>        public BigDecimal getQuantityLeftInActions() { return this.quantityLeftInActions; }
>>> +        public Map<ShoppingCartItem,BigDecimal> getUsageInfoMap() { return this.usageInfoMap; }
>>> +        public BigDecimal getUsageWeight() {
>>> +            Iterator<ShoppingCartItem> lineItems = this.usageInfoMap.keySet().iterator();
>>> +            BigDecimal totalAmount = BigDecimal.ZERO;
>>> +            while (lineItems.hasNext()) {
>>> +                ShoppingCartItem lineItem = lineItems.next();
>>> +                totalAmount = totalAmount.add(lineItem.getBasePrice().multiply(usageInfoMap.get(lineItem)));
>>> +            }
>>> +            if (totalAmount.compareTo(BigDecimal.ZERO) == 0) {
>>> +                return BigDecimal.ZERO;
>>> +            } else {
>>> +                return getTotalDiscountAmount().negate().divide(totalAmount);
>>> +            }
>>> +        }
>>> +
>>> +        public int compareTo(ProductPromoUseInfo other) {
>>> +            return other.getUsageWeight().compareTo(getUsageWeight());
>>> +        }
>>>    }
>>>
>>>    public static class CartShipInfo implements Serializable {
>>>
>>> Modified: ofbiz/trunk/applications/order/src/org/ofbiz/order/shoppingcart/ShoppingCartServices.java
>>> URL: http://svn.apache.org/viewvc/ofbiz/trunk/applications/order/src/org/ofbiz/order/shoppingcart/ShoppingCartServices.java?rev=1554265&r1=1554264&r2=1554265&view=diff
>>> ==============================================================================
>>> --- ofbiz/trunk/applications/order/src/org/ofbiz/order/shoppingcart/ShoppingCartServices.java (original)
>>> +++ ofbiz/trunk/applications/order/src/org/ofbiz/order/shoppingcart/ShoppingCartServices.java Mon Dec 30 15:53:07 2013
>>> @@ -21,6 +21,7 @@ package org.ofbiz.order.shoppingcart;
>>> import java.math.BigDecimal;
>>> import java.math.MathContext;
>>> import java.sql.Timestamp;
>>> +import java.util.HashMap;
>>> import java.util.Iterator;
>>> import java.util.List;
>>> import java.util.Locale;
>>> @@ -629,7 +630,7 @@ public class ShoppingCartServices {
>>>                cart.addProductPromoCode(productPromoCode, dispatcher);
>>>            }
>>>            for (GenericValue productPromoUse: orh.getProductPromoUse()) {
>>> -                cart.addProductPromoUse(productPromoUse.getString("productPromoId"), productPromoUse.getString("productPromoCodeId"), productPromoUse.getBigDecimal("totalDiscountAmount"), productPromoUse.getBigDecimal("quantityLeftInActions"));
>>> +                cart.addProductPromoUse(productPromoUse.getString("productPromoId"), productPromoUse.getString("productPromoCodeId"), productPromoUse.getBigDecimal("totalDiscountAmount"), productPromoUse.getBigDecimal("quantityLeftInActions"), new HashMap<ShoppingCartItem, BigDecimal>());
>>>            }
>>>        }
>>>
>>>
>>> Modified: ofbiz/trunk/applications/order/src/org/ofbiz/order/shoppingcart/product/ProductPromoWorker.java
>>> URL: http://svn.apache.org/viewvc/ofbiz/trunk/applications/order/src/org/ofbiz/order/shoppingcart/product/ProductPromoWorker.java?rev=1554265&r1=1554264&r2=1554265&view=diff
>>> ==============================================================================
>>> --- ofbiz/trunk/applications/order/src/org/ofbiz/order/shoppingcart/product/ProductPromoWorker.java (original)
>>> +++ ofbiz/trunk/applications/order/src/org/ofbiz/order/shoppingcart/product/ProductPromoWorker.java Mon Dec 30 15:53:07 2013
>>> @@ -22,6 +22,8 @@ import java.math.BigDecimal;
>>> import java.math.MathContext;
>>> import java.sql.Timestamp;
>>> import java.util.ArrayList;
>>> +import java.util.Collections;
>>> +import java.util.HashMap;
>>> import java.util.Iterator;
>>> import java.util.List;
>>> import java.util.Locale;
>>> @@ -51,6 +53,7 @@ import org.ofbiz.entity.condition.Entity
>>> import org.ofbiz.entity.util.EntityUtil;
>>> import org.ofbiz.order.shoppingcart.CartItemModifyException;
>>> import org.ofbiz.order.shoppingcart.ShoppingCart;
>>> +import org.ofbiz.order.shoppingcart.ShoppingCart.ProductPromoUseInfo;
>>> import org.ofbiz.order.shoppingcart.ShoppingCartEvents;
>>> import org.ofbiz.order.shoppingcart.ShoppingCartItem;
>>> import org.ofbiz.product.product.ProductContentWrapper;
>>> @@ -318,44 +321,62 @@ public class ProductPromoWorker {
>>>            // do a calculate only run through the promotions, then order by descending totalDiscountAmount for each promotion
>>>            // NOTE: on this run, with isolatedTestRun passed as false it should not apply any adjustments
>>>            //  or track which cart items are used for which promotions, but it will track ProductPromoUseInfo and
>>> -            //  useLimits; we are basicly just trying to run each promo "independently" to see how much each is worth
>>> +            //  useLimits; we are basically just trying to run each promo "independently" to see how much each is worth
>>>            runProductPromos(productPromoList, cart, delegator, dispatcher, nowTimestamp, true);
>>>
>>> -            // NOTE: after that first pass we could remove any that have a 0 totalDiscountAmount from the run list, but we won't because by the time they are run the cart may have changed enough to get them to go; also, certain actions like free shipping should always be run even though we won't know what the totalDiscountAmount is at the time the promotion is run
>>> -            // each ProductPromoUseInfo on the shopping cart will contain it's total value, so add up all totals for each promoId and put them in a List of Maps
>>> -            // create a List of Maps with productPromo and totalDiscountAmount, use the Map sorter to sort them descending by totalDiscountAmount
>>> -
>>> -            // before sorting split into two lists and sort each list; one list for promos that have a order total condition, and the other list for all promos that don't; then we'll always run the ones that have no condition on the order total first
>>> -            List<Map<Object, Object>> productPromoDiscountMapList = FastList.newInstance();
>>> -            List<Map<Object, Object>> productPromoDiscountMapListOrderTotal = FastList.newInstance();
>>> +            // NOTE: we can easily recognize the promos for the order total: they are the ones with usage set to 0
>>> +            Iterator<ProductPromoUseInfo> promoUses = cart.getProductPromoUseInfoIter();
>>> +            List<ProductPromoUseInfo> sortedPromoUses = new ArrayList<ProductPromoUseInfo>();
>>> +            while (promoUses.hasNext()) {
>>> +                ProductPromoUseInfo promoUse = promoUses.next();
>>> +                sortedPromoUses.add(promoUse);
>>> +            }
>>> +            Collections.sort(sortedPromoUses);
>>> +            List<GenericValue> sortedExplodedProductPromoList = new ArrayList<GenericValue>(sortedPromoUses.size());
>>> +            Map<String, Long> usesPerPromo = new HashMap<String, Long>();
>>> +            int indexOfFirstOrderTotalPromo = -1;
>>> +            for (ProductPromoUseInfo promoUse: sortedPromoUses) {
>>> +                GenericValue productPromo = delegator.findOne("ProductPromo", UtilMisc.toMap("productPromoId", promoUse.getProductPromoId()), true);
>>> +                GenericValue newProductPromo = (GenericValue)productPromo.clone();
>>> +                if (!usesPerPromo.containsKey(promoUse.getProductPromoId())) {
>>> +                    usesPerPromo.put(promoUse.getProductPromoId(), 0l);
>>> +                }
>>> +                long uses = usesPerPromo.get(promoUse.getProductPromoId());
>>> +                uses = uses + 1;
>>> +                long useLimitPerOrder = (newProductPromo.get("useLimitPerOrder") != null? newProductPromo.getLong("useLimitPerOrder"): -1);
>>> +                if (useLimitPerOrder == -1 || uses < useLimitPerOrder) {
>>> +                    newProductPromo.set("useLimitPerOrder", uses);
>>> +                }
>>> +                usesPerPromo.put(promoUse.getProductPromoId(), uses);
>>> +                sortedExplodedProductPromoList.add(newProductPromo);
>>> +                if (indexOfFirstOrderTotalPromo == -1 && BigDecimal.ZERO.equals(promoUse.getUsageWeight())) {
>>> +                    indexOfFirstOrderTotalPromo = sortedExplodedProductPromoList.size() - 1;
>>> +                }
>>> +            }
>>> +            if (indexOfFirstOrderTotalPromo == -1) {
>>> +                indexOfFirstOrderTotalPromo = sortedExplodedProductPromoList.size() - 1;
>>> +            }
>>> +
>>>            for (GenericValue productPromo : productPromoList) {
>>>                Map<Object, Object> productPromoDiscountMap = UtilGenerics.checkMap(UtilMisc.toMap("productPromo", productPromo, "totalDiscountAmount", cart.getProductPromoUseTotalDiscount(productPromo.getString("productPromoId"))));
>>>                if (hasOrderTotalCondition(productPromo, delegator)) {
>>> -                    productPromoDiscountMapListOrderTotal.add(productPromoDiscountMap);
>>> +                    if (!usesPerPromo.containsKey(productPromo.getString("productPromoId"))) {
>>> +                        sortedExplodedProductPromoList.add(productPromo);
>>> +                    }
>>>                } else {
>>> -                    productPromoDiscountMapList.add(productPromoDiscountMap);
>>> +                    if (!usesPerPromo.containsKey(productPromo.getString("productPromoId"))) {
>>> +                        if (indexOfFirstOrderTotalPromo != -1) {
>>> +                            sortedExplodedProductPromoList.add(indexOfFirstOrderTotalPromo, productPromo);
>>> +                        } else {
>>> +                            sortedExplodedProductPromoList.add(0, productPromo);
>>> +                        }
>>> +                    }
>>>                }
>>>            }
>>>
>>> -
>>> -            // sort the Map List, do it ascending because the discount amounts will be negative, so the lowest number is really the highest discount
>>> -            productPromoDiscountMapList = UtilMisc.sortMaps(productPromoDiscountMapList, UtilMisc.toList("+totalDiscountAmount"));
>>> -            productPromoDiscountMapListOrderTotal = UtilMisc.sortMaps(productPromoDiscountMapListOrderTotal, UtilMisc.toList("+totalDiscountAmount"));
>>> -
>>> -            productPromoDiscountMapList.addAll(productPromoDiscountMapListOrderTotal);
>>> -
>>> -            List<GenericValue> sortedProductPromoList = new ArrayList<GenericValue>(productPromoDiscountMapList.size());
>>> -            Iterator<Map<Object, Object>> productPromoDiscountMapIter = productPromoDiscountMapList.iterator();
>>> -            while (productPromoDiscountMapIter.hasNext()) {
>>> -                Map<Object, Object> productPromoDiscountMap = UtilGenerics.checkMap(productPromoDiscountMapIter.next());
>>> -                GenericValue productPromo = (GenericValue) productPromoDiscountMap.get("productPromo");
>>> -                sortedProductPromoList.add(productPromo);
>>> -                if (Debug.verboseOn()) Debug.logVerbose("Sorted Promo [" + productPromo.getString("productPromoId") + "] with total discount: " + productPromoDiscountMap.get("totalDiscountAmount"), module);
>>> -            }
>>> -
>>>            // okay, all ready, do the real run, clearing the temporary result first...
>>>            cart.clearAllPromotionInformation();
>>> -            runProductPromos(sortedProductPromoList, cart, delegator, dispatcher, nowTimestamp, false);
>>> +            runProductPromos(sortedExplodedProductPromoList, cart, delegator, dispatcher, nowTimestamp, false);
>>>        } catch (NumberFormatException e) {
>>>            Debug.logError(e, "Number not formatted correctly in promotion rules, not completed...", module);
>>>        } catch (GenericEntityException e) {
>>> @@ -436,7 +457,7 @@ public class ProductPromoWorker {
>>>                                    GenericValue productPromoCode = productPromoCodeIter.next();
>>>                                    String productPromoCodeId = productPromoCode.getString("productPromoCodeId");
>>>                                    Long codeUseLimit = getProductPromoCodeUseLimit(productPromoCode, partyId, delegator);
>>> -                                    if (runProductPromoRules(cart, cartChanged, useLimit, true, productPromoCodeId, codeUseLimit, maxUseLimit, productPromo, productPromoRules, dispatcher, delegator, nowTimestamp)) {
>>> +                                    if (runProductPromoRules(cart, useLimit, true, productPromoCodeId, codeUseLimit, maxUseLimit, productPromo, productPromoRules, dispatcher, delegator, nowTimestamp)) {
>>>                                        cartChanged = true;
>>>                                    }
>>>
>>> @@ -448,7 +469,7 @@ public class ProductPromoWorker {
>>>                            }
>>>                        } else {
>>>                            try {
>>> -                                if (runProductPromoRules(cart, cartChanged, useLimit, false, null, null, maxUseLimit, productPromo, productPromoRules, dispatcher, delegator, nowTimestamp)) {
>>> +                                if (runProductPromoRules(cart, useLimit, false, null, null, maxUseLimit, productPromo, productPromoRules, dispatcher, delegator, nowTimestamp)) {
>>>                                    cartChanged = true;
>>>                                }
>>>                            } catch (RuntimeException e) {
>>> @@ -735,8 +756,10 @@ public class ProductPromoWorker {
>>>        return promoDescBuf.toString();
>>>    }
>>>
>>> -    protected static boolean runProductPromoRules(ShoppingCart cart, boolean cartChanged, Long useLimit, boolean requireCode, String productPromoCodeId, Long codeUseLimit, long maxUseLimit,
>>> +    protected static boolean runProductPromoRules(ShoppingCart cart, Long useLimit, boolean requireCode, String productPromoCodeId, Long codeUseLimit, long maxUseLimit,
>>>        GenericValue productPromo, List<GenericValue> productPromoRules, LocalDispatcher dispatcher, Delegator delegator, Timestamp nowTimestamp) throws GenericEntityException, UseLimitException {
>>> +        boolean cartChanged = false;
>>> +        Map<ShoppingCartItem,BigDecimal> usageInfoMap = prepareProductUsageInfoMap(cart);
>>>        String productPromoId = productPromo.getString("productPromoId");
>>>        while ((useLimit == null || useLimit.longValue() > cart.getProductPromoUseCount(productPromoId)) &&
>>>                (!requireCode || UtilValidate.isNotEmpty(productPromoCodeId)) &&
>>> @@ -755,17 +778,17 @@ public class ProductPromoWorker {
>>>                // loop through conditions for rule, if any false, set allConditionsTrue to false
>>>                List<GenericValue> productPromoConds = delegator.findByAnd("ProductPromoCond", UtilMisc.toMap("productPromoId", productPromo.get("productPromoId")), UtilMisc.toList("productPromoCondSeqId"), true);
>>>                productPromoConds = EntityUtil.filterByAnd(productPromoConds, UtilMisc.toMap("productPromoRuleId", productPromoRule.get("productPromoRuleId")));
>>> -                // using the other method to consolodate cache entries because the same cache is used elsewhere: List productPromoConds = productPromoRule.getRelated("ProductPromoCond", null, UtilMisc.toList("productPromoCondSeqId"), true);
>>> +                // using the other method to consolidate cache entries because the same cache is used elsewhere: List productPromoConds = productPromoRule.getRelated("ProductPromoCond", null, UtilMisc.toList("productPromoCondSeqId"), true);
>>>                if (Debug.verboseOn()) Debug.logVerbose("Checking " + productPromoConds.size() + " conditions for rule " + productPromoRule, module);
>>>
>>>                Iterator<GenericValue> productPromoCondIter = UtilMisc.toIterator(productPromoConds);
>>>                while (productPromoCondIter != null && productPromoCondIter.hasNext()) {
>>>                    GenericValue productPromoCond = productPromoCondIter.next();
>>>
>>> -                    boolean condResult = checkCondition(productPromoCond, cart, delegator, dispatcher, nowTimestamp);
>>> +                    boolean conditionSatisfied = checkCondition(productPromoCond, cart, delegator, dispatcher, nowTimestamp);
>>>
>>>                    // any false condition will cause it to NOT perform the action
>>> -                    if (condResult == false) {
>>> +                    if (!conditionSatisfied) {
>>>                        performActions = false;
>>>                        break;
>>>                    }
>>> @@ -797,13 +820,16 @@ public class ProductPromoWorker {
>>>            }
>>>
>>>            if (promoUsed) {
>>> -                cart.addProductPromoUse(productPromo.getString("productPromoId"), productPromoCodeId, totalDiscountAmount, quantityLeftInActions);
>>> +                // Get product use information from the cart
>>> +                Map<ShoppingCartItem,BigDecimal> newUsageInfoMap = prepareProductUsageInfoMap(cart);
>>> +                Map<ShoppingCartItem,BigDecimal> deltaUsageInfoMap = prepareDeltaProductUsageInfoMap(usageInfoMap, newUsageInfoMap);
>>> +                usageInfoMap = newUsageInfoMap;
>>> +                cart.addProductPromoUse(productPromo.getString("productPromoId"), productPromoCodeId, totalDiscountAmount, quantityLeftInActions, deltaUsageInfoMap);
>>>            } else {
>>>                // the promotion was not used, don't try again until we finish a full pass and come back to see the promo conditions are now satisfied based on changes to the cart
>>>                break;
>>>            }
>>>
>>> -
>>>            if (cart.getProductPromoUseCount(productPromoId) > maxUseLimit) {
>>>                throw new UseLimitException("ERROR: While calculating promotions the promotion [" + productPromoId + "] action was applied more than " + maxUseLimit + " times, so the calculation has been ended. This should generally never happen unless you have bad rule definitions.");
>>>            }
>>> @@ -812,6 +838,34 @@ public class ProductPromoWorker {
>>>        return cartChanged;
>>>    }
>>>
>>> +    private static Map<ShoppingCartItem,BigDecimal> prepareProductUsageInfoMap(ShoppingCart cart) {
>>> +        Map<ShoppingCartItem,BigDecimal> usageInfoMap = new HashMap<ShoppingCartItem, BigDecimal>();
>>> +        List<ShoppingCartItem> lineOrderedByBasePriceList = cart.getLineListOrderedByBasePrice(false);
>>> +        for (ShoppingCartItem cartItem : lineOrderedByBasePriceList) {
>>> +            BigDecimal used = cartItem.getPromoQuantityUsed();
>>> +            if (used.compareTo(BigDecimal.ZERO) != 0) {
>>> +                usageInfoMap.put(cartItem, used);
>>> +            }
>>> +        }
>>> +        return usageInfoMap;
>>> +    }
>>> +
>>> +    private static Map<ShoppingCartItem,BigDecimal> prepareDeltaProductUsageInfoMap(Map<ShoppingCartItem,BigDecimal> oldMap, Map<ShoppingCartItem,BigDecimal> newMap) {
>>> +        Map<ShoppingCartItem,BigDecimal> deltaUsageInfoMap = new HashMap<ShoppingCartItem, BigDecimal>(newMap);
>>> +        Iterator<ShoppingCartItem> cartLines = oldMap.keySet().iterator();
>>> +        while (cartLines.hasNext()) {
>>> +            ShoppingCartItem cartLine = cartLines.next();
>>> +            BigDecimal oldUsed = oldMap.get(cartLine);
>>> +            BigDecimal newUsed = newMap.get(cartLine);
>>> +            if (newUsed.compareTo(oldUsed) > 0) {
>>> +                deltaUsageInfoMap.put(cartLine, newUsed.add(oldUsed.negate()));
>>> +            } else {
>>> +                deltaUsageInfoMap.remove(cartLine);
>>> +            }
>>> +        }
>>> +        return deltaUsageInfoMap;
>>> +    }
>>> +
>>>    protected static boolean checkCondition(GenericValue productPromoCond, ShoppingCart cart, Delegator delegator, LocalDispatcher dispatcher, Timestamp nowTimestamp) throws GenericEntityException {
>>>        String condValue = productPromoCond.getString("condValue");
>>>        String otherValue = productPromoCond.getString("otherValue");
>>> @@ -1772,8 +1826,8 @@ public class ProductPromoWorker {
>>>            actionResultInfo.ranAction = false;
>>>        }
>>>
>>> +        // in action, if doesn't have enough quantity to use the promo at all, remove candidate promo uses and increment promoQuantityUsed; this should go for all actions, if any action runs we confirm
>>>        if (actionResultInfo.ranAction) {
>>> -            // in action, if doesn't have enough quantity to use the promo at all, remove candidate promo uses and increment promoQuantityUsed; this should go for all actions, if any action runs we confirm
>>>            cart.confirmPromoRuleUse(productPromoAction.getString("productPromoId"), productPromoAction.getString("productPromoRuleId"));
>>>        } else {
>>>            cart.resetPromoRuleUse(productPromoAction.getString("productPromoId"), productPromoAction.getString("productPromoRuleId"));
>>>
>>>
>>>
>>
>>
>>
>

Reply | Threaded
Open this post in threaded view
|

Re: svn commit: r1554265 - in /ofbiz/trunk/applications/order/src/org/ofbiz/order/shoppingcart: ShoppingCart.java ShoppingCartServices.java product/ProductPromoWorker.java

Adrian Crum-3
Thanks Jacopo!

-Adrian


Quoting Jacopo Cappellato <[hidden email]>:

> Fixed at rev. 1558145
> Thanks for the report.
>
> Jacopo
>
> On Jan 14, 2014, at 6:36 PM, Jacopo Cappellato  
> <[hidden email]> wrote:
>
>> Thanks Adrian,
>>
>> I am looking into this.
>>
>> Jacopo
>>
>> On Jan 14, 2014, at 6:24 PM, [hidden email] wrote:
>>
>>> This commit breaks promotions.
>>>
>>> 1. Create a new sales order, customer DemoCustomer.
>>> 2. Add GZ-8544, qty 1 to the order.
>>> 3. Promo items do not appear in order.
>>> 4. Log shows an exception being thrown:
>>>
>>> 2014-01-14 12:15:56,030 (http-bio-0.0.0.0-8443-exec-1) [  
>>> ProductPromoWorker.java:385:ERROR]
>>> ---- runtime exception report  
>>> --------------------------------------------------
>>> Error running promotions, will ignore:  
>>> java.lang.ArithmeticException: Non-terminating decimal expansion;  
>>> no exact representable decimal result.
>>> Exception: java.lang.ArithmeticException
>>> Message: Non-terminating decimal expansion; no exact representable  
>>> decimal result.
>>> ---- stack trace  
>>> ---------------------------------------------------------------
>>> java.lang.ArithmeticException: Non-terminating decimal expansion;  
>>> no exact representable decimal result.
>>> java.math.BigDecimal.divide(BigDecimal.java:1616)
>>> org.ofbiz.order.shoppingcart.ShoppingCart$ProductPromoUseInfo.getUsageWeight(ShoppingCart.java:4418)
>>> org.ofbiz.order.shoppingcart.ShoppingCart$ProductPromoUseInfo.compareTo(ShoppingCart.java:4423)
>>> org.ofbiz.order.shoppingcart.ShoppingCart$ProductPromoUseInfo.compareTo(ShoppingCart.java:4388)
>>> java.util.ComparableTimSort.countRunAndMakeAscending(ComparableTimSort.java:290)
>>> java.util.ComparableTimSort.sort(ComparableTimSort.java:157)
>>> java.util.ComparableTimSort.sort(ComparableTimSort.java:146)
>>> java.util.Arrays.sort(Arrays.java:472)
>>> java.util.Collections.sort(Collections.java:155)
>>> org.ofbiz.order.shoppingcart.product.ProductPromoWorker.doPromotions(ProductPromoWorker.java:334)
>>> org.ofbiz.order.shoppingcart.product.ProductPromoWorker.doPromotions(ProductPromoWorker.java:293)
>>> org.ofbiz.order.shoppingcart.ShoppingCartItem.setQuantity(ShoppingCartItem.java:1061)
>>> org.ofbiz.order.shoppingcart.ShoppingCartItem.makeItem(ShoppingCartItem.java:502)
>>> org.ofbiz.order.shoppingcart.ShoppingCartItem.makeItem(ShoppingCartItem.java:334)
>>> org.ofbiz.order.shoppingcart.ShoppingCart.addOrIncreaseItem(ShoppingCart.java:590)
>>> org.ofbiz.order.shoppingcart.ShoppingCartHelper.addToCart(ShoppingCartHelper.java:245)
>>> org.ofbiz.order.shoppingcart.ShoppingCartEvents.addToCart(ShoppingCartEvents.java:638)
>>> sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
>>> sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:57)
>>> sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
>>> java.lang.reflect.Method.invoke(Method.java:606)
>>> org.ofbiz.webapp.event.JavaEventHandler.invoke(JavaEventHandler.java:93)
>>> org.ofbiz.webapp.event.JavaEventHandler.invoke(JavaEventHandler.java:79)
>>> org.ofbiz.webapp.control.RequestHandler.runEvent(RequestHandler.java:749)
>>> org.ofbiz.webapp.control.RequestHandler.doRequest(RequestHandler.java:469)
>>> org.ofbiz.webapp.control.ControlServlet.doGet(ControlServlet.java:219)
>>> org.ofbiz.webapp.control.ControlServlet.doPost(ControlServlet.java:91)
>>> javax.servlet.http.HttpServlet.service(HttpServlet.java:641)
>>> javax.servlet.http.HttpServlet.service(HttpServlet.java:722)
>>> org.apache.catalina.core.ApplicationFilterChain.internalDoFilter(ApplicationFilterChain.java:305)
>>> org.apache.catalina.core.ApplicationFilterChain.doFilter(ApplicationFilterChain.java:210)
>>> org.ofbiz.webapp.control.ContextFilter.doFilter(ContextFilter.java:314)
>>> org.apache.catalina.core.ApplicationFilterChain.internalDoFilter(ApplicationFilterChain.java:243)
>>> org.apache.catalina.core.ApplicationFilterChain.doFilter(ApplicationFilterChain.java:210)
>>> org.apache.catalina.core.StandardWrapperValve.invoke(StandardWrapperValve.java:222)
>>> org.apache.catalina.core.StandardContextValve.invoke(StandardContextValve.java:123)
>>> org.apache.catalina.authenticator.AuthenticatorBase.invoke(AuthenticatorBase.java:502)
>>> org.apache.catalina.core.StandardHostValve.invoke(StandardHostValve.java:171)
>>> org.apache.catalina.valves.ErrorReportValve.invoke(ErrorReportValve.java:100)
>>> org.apache.catalina.core.StandardEngineValve.invoke(StandardEngineValve.java:118)
>>> org.apache.catalina.valves.AccessLogValve.invoke(AccessLogValve.java:953)
>>> org.apache.catalina.connector.CoyoteAdapter.service(CoyoteAdapter.java:409)
>>> org.apache.coyote.http11.AbstractHttp11Processor.process(AbstractHttp11Processor.java:1044)
>>> org.apache.coyote.AbstractProtocol$AbstractConnectionHandler.process(AbstractProtocol.java:607)
>>> org.apache.tomcat.util.net.JIoEndpoint$SocketProcessor.run(JIoEndpoint.java:315)
>>> java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1145)
>>> java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:615)
>>> java.lang.Thread.run(Thread.java:744)
>>> --------------------------------------------------------------------------------
>>>
>>>
>>> -Adrian
>>>
>>>
>>>
>>> Quoting [hidden email]:
>>>
>>>> Author: jacopoc
>>>> Date: Mon Dec 30 15:53:07 2013
>>>> New Revision: 1554265
>>>>
>>>> URL: http://svn.apache.org/r1554265
>>>> Log:
>>>> This is a refactoring of the product promotion engine in order to  
>>>> overcome some limitations that prevented it to select and apply  
>>>> the best set of promotions under special conditions.
>>>>
>>>> Example: Consider two promotions:
>>>> * consider two products: Product A, with price $20, and Product  
>>>> B, with price $40
>>>> * Promotion 1: 20% discount on all the products in a category  
>>>> containing Product A and Product B
>>>> * Promotion 2: 40% discount on Product A
>>>>
>>>> When Product A and Product B are both in the cart:
>>>> * Expected behavior: on Product A the Promotion 2 should be  
>>>> applied i.e. 40% discount, and on Product B Promotion 1 should be  
>>>> applied i.e. 20% discount.
>>>> ** Summary
>>>> Product Price Discount Subtotal
>>>> A $20 $8 (40% discount) $12
>>>> B $40 $8 (20% discount) $32
>>>> Total Adjustment: $16
>>>>
>>>> * OFBiz behavior (before this fix): Promotion 1 is applied to  
>>>> Product A and Product B; this happens because the total discount  
>>>> of Promotion 1 is greater than the total discount of Promotion 2  
>>>> and OFBiz applies promotions sorted by discount (desc)
>>>> ** Summary
>>>> Product Price Discount Subtotal
>>>> A $20 $4 (20% discount) $16
>>>> B $40 $8 (20% discount) $32
>>>> Total Adjustment: $12
>>>>
>>>> The new solution fixes this issue and similar ones.
>>>>
>>>> Here are some details about the new algorithm.
>>>>
>>>> Overview of the flow:
>>>> 1) run the promotions one by one in a test run
>>>> 2) collect the ProductPromoUse information
>>>> 3) sort them by weight (i.e. the ratio between the discount and  
>>>> the value of the products discounted)
>>>> 4) execute the ProductPromoUse in the given order
>>>>
>>>> In order to understand this solution, and specifically the  
>>>> changes to ProductPromoWorker.java, there is an important concept  
>>>> to consider:
>>>> one Promotion can generate more than one ProductPromoUseInfo objects.
>>>> For example if I have 2 units of WG-1111 in the cart (in one cart  
>>>> item) and I have the promotion “20% discount on WG-1111 and  
>>>> GZ-1000” then the system will create TWO ProductPromoUseInfo  
>>>> objects both associated to the same promotion one for each of the  
>>>> 2 units discounted.
>>>> Similarly if I had two lines: 2 units of WG-1111 and 1 unit of  
>>>> GZ-1000 I would get 3 ProductPromoUseInfo objects 2 objects for  
>>>> WG-1111 and 1 object for GZ-1000
>>>>
>>>> We can sort these ProductPromoUseInfo objects based on their  
>>>> weight (i.e. the ratio between the discount and the value of the  
>>>> products discounted) in desc order
>>>> and now we have a sorted list of ProductPromoUseInfo objects  
>>>> ready to be executed
>>>> However we only want to execute each of them once and for this  
>>>> reason we set (in memory, not in the DB) the useLimitPerOrder to  
>>>> 1 in the first ProductPromoUseInfo of a given promotion and then  
>>>> to 2 if the same ProductPromoUseInfo is associated to the same  
>>>> promotion etc...
>>>> in this way the end result is that the system will generate, as  
>>>> we desire, ONE ProductPromoUseInfo only for each of the  
>>>> ProductPromoUseInfo in the list.
>>>>
>>>> Here is an example:
>>>> we have 2 promotions:
>>>> PROMO A
>>>> PROMO B
>>>>
>>>> After test run:
>>>>
>>>> ProductPromoUseInfo - PROMO A - #1 - weight 0.3
>>>> ProductPromoUseInfo - PROMO A - #2 - weight 0.3
>>>> ProductPromoUseInfo - PROMO B - #1 - weight 0.4
>>>>
>>>> After sorting:
>>>>
>>>> ProductPromoUseInfo - PROMO B - #1 - weight 0.4
>>>> ProductPromoUseInfo - PROMO A - #1 - weight 0.3
>>>> ProductPromoUseInfo - PROMO A - #2 - weight 0.3
>>>>
>>>> Based on this we create a list (sortedExplodedProductPromoList)  
>>>> of ProductPromo:
>>>>
>>>> PROMO B - with useLimitPerOrder=1
>>>> PROMO A - with useLimitPerOrder=1
>>>> PROMO A - with useLimitPerOrder=2
>>>>
>>>> When we apply these to the cart we get the following results:
>>>>
>>>> PROMO B - with useLimitPerOrder=1 APPLIED
>>>> PROMO A - with useLimitPerOrder=1 APPLIED
>>>> PROMO A - with useLimitPerOrder=2 NOT APPLIED (because PROMO B  
>>>> used the item)
>>>>
>>>>
>>>> Modified:
>>>>    
>>>> ofbiz/trunk/applications/order/src/org/ofbiz/order/shoppingcart/ShoppingCart.java
>>>>    
>>>> ofbiz/trunk/applications/order/src/org/ofbiz/order/shoppingcart/ShoppingCartServices.java
>>>>    
>>>> ofbiz/trunk/applications/order/src/org/ofbiz/order/shoppingcart/product/ProductPromoWorker.java
>>>>
>>>> Modified:  
>>>> ofbiz/trunk/applications/order/src/org/ofbiz/order/shoppingcart/ShoppingCart.java
>>>> URL:  
>>>> http://svn.apache.org/viewvc/ofbiz/trunk/applications/order/src/org/ofbiz/order/shoppingcart/ShoppingCart.java?rev=1554265&r1=1554264&r2=1554265&view=diff
>>>> ==============================================================================
>>>> ---  
>>>> ofbiz/trunk/applications/order/src/org/ofbiz/order/shoppingcart/ShoppingCart.java  
>>>> (original)
>>>> +++  
>>>> ofbiz/trunk/applications/order/src/org/ofbiz/order/shoppingcart/ShoppingCart.java Mon Dec 30 15:53:07  
>>>> 2013
>>>> @@ -2714,7 +2714,7 @@ public class ShoppingCart implements Ite
>>>>            }
>>>>            itemsTotal = itemsTotal.add(cartItem.getItemSubTotal());
>>>>        }
>>>> -        return itemsTotal;
>>>> +        return itemsTotal.add(this.getOrderOtherAdjustmentTotal());
>>>>    }
>>>>
>>>>    /**
>>>> @@ -3142,12 +3142,12 @@ public class ShoppingCart implements Ite
>>>>        return new HashMap<GenericPK,  
>>>> String>(this.desiredAlternateGiftByAction);
>>>>    }
>>>>
>>>> -    public void addProductPromoUse(String productPromoId, String  
>>>> productPromoCodeId, BigDecimal totalDiscountAmount, BigDecimal  
>>>> quantityLeftInActions) {
>>>> +    public void addProductPromoUse(String productPromoId, String  
>>>> productPromoCodeId, BigDecimal totalDiscountAmount, BigDecimal  
>>>> quantityLeftInActions, Map<ShoppingCartItem,BigDecimal>  
>>>> usageInfoMap) {
>>>>        if (UtilValidate.isNotEmpty(productPromoCodeId) &&  
>>>> !this.productPromoCodes.contains(productPromoCodeId)) {
>>>>            throw new IllegalStateException("Cannot add a use to a  
>>>> promo code use for a code that has not been entered.");
>>>>        }
>>>>        if (Debug.verboseOn()) Debug.logVerbose("Used promotion ["  
>>>> + productPromoId + "] with code [" + productPromoCodeId + "] for  
>>>> total discount [" + totalDiscountAmount + "] and quantity left in  
>>>> actions [" + quantityLeftInActions + "]", module);
>>>> -        this.productPromoUseInfoList.add(new  
>>>> ProductPromoUseInfo(productPromoId, productPromoCodeId,  
>>>> totalDiscountAmount, quantityLeftInActions));
>>>> +        this.productPromoUseInfoList.add(new  
>>>> ProductPromoUseInfo(productPromoId, productPromoCodeId,  
>>>> totalDiscountAmount, quantityLeftInActions, usageInfoMap));
>>>>    }
>>>>
>>>>    public void removeProductPromoUse(String productPromoId) {
>>>> @@ -4385,23 +4385,43 @@ public class ShoppingCart implements Ite
>>>>        }
>>>>    }
>>>>
>>>> -    public static class ProductPromoUseInfo implements Serializable {
>>>> +    public static class ProductPromoUseInfo implements  
>>>> Serializable, Comparable<ProductPromoUseInfo> {
>>>>        public String productPromoId = null;
>>>>        public String productPromoCodeId = null;
>>>>        public BigDecimal totalDiscountAmount = BigDecimal.ZERO;
>>>>        public BigDecimal quantityLeftInActions = BigDecimal.ZERO;
>>>> +        private Map<ShoppingCartItem,BigDecimal> usageInfoMap = null;
>>>>
>>>> -        public ProductPromoUseInfo(String productPromoId, String  
>>>> productPromoCodeId, BigDecimal totalDiscountAmount, BigDecimal  
>>>> quantityLeftInActions) {
>>>> +        public ProductPromoUseInfo(String productPromoId, String  
>>>> productPromoCodeId, BigDecimal totalDiscountAmount, BigDecimal  
>>>> quantityLeftInActions, Map<ShoppingCartItem,BigDecimal>  
>>>> usageInfoMap) {
>>>>            this.productPromoId = productPromoId;
>>>>            this.productPromoCodeId = productPromoCodeId;
>>>>            this.totalDiscountAmount = totalDiscountAmount;
>>>>            this.quantityLeftInActions = quantityLeftInActions;
>>>> +            this.usageInfoMap = usageInfoMap;
>>>>        }
>>>>
>>>>        public String getProductPromoId() { return this.productPromoId; }
>>>>        public String getProductPromoCodeId() { return  
>>>> this.productPromoCodeId; }
>>>>        public BigDecimal getTotalDiscountAmount() { return  
>>>> this.totalDiscountAmount; }
>>>>        public BigDecimal getQuantityLeftInActions() { return  
>>>> this.quantityLeftInActions; }
>>>> +        public Map<ShoppingCartItem,BigDecimal>  
>>>> getUsageInfoMap() { return this.usageInfoMap; }
>>>> +        public BigDecimal getUsageWeight() {
>>>> +            Iterator<ShoppingCartItem> lineItems =  
>>>> this.usageInfoMap.keySet().iterator();
>>>> +            BigDecimal totalAmount = BigDecimal.ZERO;
>>>> +            while (lineItems.hasNext()) {
>>>> +                ShoppingCartItem lineItem = lineItems.next();
>>>> +                totalAmount =  
>>>> totalAmount.add(lineItem.getBasePrice().multiply(usageInfoMap.get(lineItem)));
>>>> +            }
>>>> +            if (totalAmount.compareTo(BigDecimal.ZERO) == 0) {
>>>> +                return BigDecimal.ZERO;
>>>> +            } else {
>>>> +                return  
>>>> getTotalDiscountAmount().negate().divide(totalAmount);
>>>> +            }
>>>> +        }
>>>> +
>>>> +        public int compareTo(ProductPromoUseInfo other) {
>>>> +            return other.getUsageWeight().compareTo(getUsageWeight());
>>>> +        }
>>>>    }
>>>>
>>>>    public static class CartShipInfo implements Serializable {
>>>>
>>>> Modified:  
>>>> ofbiz/trunk/applications/order/src/org/ofbiz/order/shoppingcart/ShoppingCartServices.java
>>>> URL:  
>>>> http://svn.apache.org/viewvc/ofbiz/trunk/applications/order/src/org/ofbiz/order/shoppingcart/ShoppingCartServices.java?rev=1554265&r1=1554264&r2=1554265&view=diff
>>>> ==============================================================================
>>>> ---  
>>>> ofbiz/trunk/applications/order/src/org/ofbiz/order/shoppingcart/ShoppingCartServices.java  
>>>> (original)
>>>> +++  
>>>> ofbiz/trunk/applications/order/src/org/ofbiz/order/shoppingcart/ShoppingCartServices.java Mon Dec 30 15:53:07  
>>>> 2013
>>>> @@ -21,6 +21,7 @@ package org.ofbiz.order.shoppingcart;
>>>> import java.math.BigDecimal;
>>>> import java.math.MathContext;
>>>> import java.sql.Timestamp;
>>>> +import java.util.HashMap;
>>>> import java.util.Iterator;
>>>> import java.util.List;
>>>> import java.util.Locale;
>>>> @@ -629,7 +630,7 @@ public class ShoppingCartServices {
>>>>                cart.addProductPromoCode(productPromoCode, dispatcher);
>>>>            }
>>>>            for (GenericValue productPromoUse: orh.getProductPromoUse()) {
>>>> -                
>>>> cart.addProductPromoUse(productPromoUse.getString("productPromoId"),  
>>>> productPromoUse.getString("productPromoCodeId"),  
>>>> productPromoUse.getBigDecimal("totalDiscountAmount"),  
>>>> productPromoUse.getBigDecimal("quantityLeftInActions"));
>>>> +                
>>>> cart.addProductPromoUse(productPromoUse.getString("productPromoId"),  
>>>> productPromoUse.getString("productPromoCodeId"),  
>>>> productPromoUse.getBigDecimal("totalDiscountAmount"),  
>>>> productPromoUse.getBigDecimal("quantityLeftInActions"), new  
>>>> HashMap<ShoppingCartItem, BigDecimal>());
>>>>            }
>>>>        }
>>>>
>>>>
>>>> Modified:  
>>>> ofbiz/trunk/applications/order/src/org/ofbiz/order/shoppingcart/product/ProductPromoWorker.java
>>>> URL:  
>>>> http://svn.apache.org/viewvc/ofbiz/trunk/applications/order/src/org/ofbiz/order/shoppingcart/product/ProductPromoWorker.java?rev=1554265&r1=1554264&r2=1554265&view=diff
>>>> ==============================================================================
>>>> ---  
>>>> ofbiz/trunk/applications/order/src/org/ofbiz/order/shoppingcart/product/ProductPromoWorker.java  
>>>> (original)
>>>> +++  
>>>> ofbiz/trunk/applications/order/src/org/ofbiz/order/shoppingcart/product/ProductPromoWorker.java Mon Dec 30 15:53:07  
>>>> 2013
>>>> @@ -22,6 +22,8 @@ import java.math.BigDecimal;
>>>> import java.math.MathContext;
>>>> import java.sql.Timestamp;
>>>> import java.util.ArrayList;
>>>> +import java.util.Collections;
>>>> +import java.util.HashMap;
>>>> import java.util.Iterator;
>>>> import java.util.List;
>>>> import java.util.Locale;
>>>> @@ -51,6 +53,7 @@ import org.ofbiz.entity.condition.Entity
>>>> import org.ofbiz.entity.util.EntityUtil;
>>>> import org.ofbiz.order.shoppingcart.CartItemModifyException;
>>>> import org.ofbiz.order.shoppingcart.ShoppingCart;
>>>> +import org.ofbiz.order.shoppingcart.ShoppingCart.ProductPromoUseInfo;
>>>> import org.ofbiz.order.shoppingcart.ShoppingCartEvents;
>>>> import org.ofbiz.order.shoppingcart.ShoppingCartItem;
>>>> import org.ofbiz.product.product.ProductContentWrapper;
>>>> @@ -318,44 +321,62 @@ public class ProductPromoWorker {
>>>>            // do a calculate only run through the promotions,  
>>>> then order by descending totalDiscountAmount for each promotion
>>>>            // NOTE: on this run, with isolatedTestRun passed as  
>>>> false it should not apply any adjustments
>>>>            //  or track which cart items are used for which  
>>>> promotions, but it will track ProductPromoUseInfo and
>>>> -            //  useLimits; we are basicly just trying to run  
>>>> each promo "independently" to see how much each is worth
>>>> +            //  useLimits; we are basically just trying to run  
>>>> each promo "independently" to see how much each is worth
>>>>            runProductPromos(productPromoList, cart, delegator,  
>>>> dispatcher, nowTimestamp, true);
>>>>
>>>> -            // NOTE: after that first pass we could remove any  
>>>> that have a 0 totalDiscountAmount from the run list, but we won't  
>>>> because by the time they are run the cart may have changed enough  
>>>> to get them to go; also, certain actions like free shipping  
>>>> should always be run even though we won't know what the  
>>>> totalDiscountAmount is at the time the promotion is run
>>>> -            // each ProductPromoUseInfo on the shopping cart  
>>>> will contain it's total value, so add up all totals for each  
>>>> promoId and put them in a List of Maps
>>>> -            // create a List of Maps with productPromo and  
>>>> totalDiscountAmount, use the Map sorter to sort them descending  
>>>> by totalDiscountAmount
>>>> -
>>>> -            // before sorting split into two lists and sort each  
>>>> list; one list for promos that have a order total condition, and  
>>>> the other list for all promos that don't; then we'll always run  
>>>> the ones that have no condition on the order total first
>>>> -            List<Map<Object, Object>>  
>>>> productPromoDiscountMapList = FastList.newInstance();
>>>> -            List<Map<Object, Object>>  
>>>> productPromoDiscountMapListOrderTotal = FastList.newInstance();
>>>> +            // NOTE: we can easily recognize the promos for the  
>>>> order total: they are the ones with usage set to 0
>>>> +            Iterator<ProductPromoUseInfo> promoUses =  
>>>> cart.getProductPromoUseInfoIter();
>>>> +            List<ProductPromoUseInfo> sortedPromoUses = new  
>>>> ArrayList<ProductPromoUseInfo>();
>>>> +            while (promoUses.hasNext()) {
>>>> +                ProductPromoUseInfo promoUse = promoUses.next();
>>>> +                sortedPromoUses.add(promoUse);
>>>> +            }
>>>> +            Collections.sort(sortedPromoUses);
>>>> +            List<GenericValue> sortedExplodedProductPromoList =  
>>>> new ArrayList<GenericValue>(sortedPromoUses.size());
>>>> +            Map<String, Long> usesPerPromo = new HashMap<String, Long>();
>>>> +            int indexOfFirstOrderTotalPromo = -1;
>>>> +            for (ProductPromoUseInfo promoUse: sortedPromoUses) {
>>>> +                GenericValue productPromo =  
>>>> delegator.findOne("ProductPromo",  
>>>> UtilMisc.toMap("productPromoId", promoUse.getProductPromoId()),  
>>>> true);
>>>> +                GenericValue newProductPromo =  
>>>> (GenericValue)productPromo.clone();
>>>> +                if  
>>>> (!usesPerPromo.containsKey(promoUse.getProductPromoId())) {
>>>> +                    usesPerPromo.put(promoUse.getProductPromoId(), 0l);
>>>> +                }
>>>> +                long uses =  
>>>> usesPerPromo.get(promoUse.getProductPromoId());
>>>> +                uses = uses + 1;
>>>> +                long useLimitPerOrder =  
>>>> (newProductPromo.get("useLimitPerOrder") != null?  
>>>> newProductPromo.getLong("useLimitPerOrder"): -1);
>>>> +                if (useLimitPerOrder == -1 || uses < useLimitPerOrder) {
>>>> +                    newProductPromo.set("useLimitPerOrder", uses);
>>>> +                }
>>>> +                usesPerPromo.put(promoUse.getProductPromoId(), uses);
>>>> +                sortedExplodedProductPromoList.add(newProductPromo);
>>>> +                if (indexOfFirstOrderTotalPromo == -1 &&  
>>>> BigDecimal.ZERO.equals(promoUse.getUsageWeight())) {
>>>> +                    indexOfFirstOrderTotalPromo =  
>>>> sortedExplodedProductPromoList.size() - 1;
>>>> +                }
>>>> +            }
>>>> +            if (indexOfFirstOrderTotalPromo == -1) {
>>>> +                indexOfFirstOrderTotalPromo =  
>>>> sortedExplodedProductPromoList.size() - 1;
>>>> +            }
>>>> +
>>>>            for (GenericValue productPromo : productPromoList) {
>>>>                Map<Object, Object> productPromoDiscountMap =  
>>>> UtilGenerics.checkMap(UtilMisc.toMap("productPromo",  
>>>> productPromo, "totalDiscountAmount",  
>>>> cart.getProductPromoUseTotalDiscount(productPromo.getString("productPromoId"))));
>>>>                if (hasOrderTotalCondition(productPromo, delegator)) {
>>>> -                    
>>>> productPromoDiscountMapListOrderTotal.add(productPromoDiscountMap);
>>>> +                    if  
>>>> (!usesPerPromo.containsKey(productPromo.getString("productPromoId")))  
>>>> {
>>>> +                        sortedExplodedProductPromoList.add(productPromo);
>>>> +                    }
>>>>                } else {
>>>> -                    
>>>> productPromoDiscountMapList.add(productPromoDiscountMap);
>>>> +                    if  
>>>> (!usesPerPromo.containsKey(productPromo.getString("productPromoId")))  
>>>> {
>>>> +                        if (indexOfFirstOrderTotalPromo != -1) {
>>>> +                            
>>>> sortedExplodedProductPromoList.add(indexOfFirstOrderTotalPromo,  
>>>> productPromo);
>>>> +                        } else {
>>>> +                            
>>>> sortedExplodedProductPromoList.add(0, productPromo);
>>>> +                        }
>>>> +                    }
>>>>                }
>>>>            }
>>>>
>>>> -
>>>> -            // sort the Map List, do it ascending because the  
>>>> discount amounts will be negative, so the lowest number is really  
>>>> the highest discount
>>>> -            productPromoDiscountMapList =  
>>>> UtilMisc.sortMaps(productPromoDiscountMapList,  
>>>> UtilMisc.toList("+totalDiscountAmount"));
>>>> -            productPromoDiscountMapListOrderTotal =  
>>>> UtilMisc.sortMaps(productPromoDiscountMapListOrderTotal,  
>>>> UtilMisc.toList("+totalDiscountAmount"));
>>>> -
>>>> -            
>>>> productPromoDiscountMapList.addAll(productPromoDiscountMapListOrderTotal);
>>>> -
>>>> -            List<GenericValue> sortedProductPromoList = new  
>>>> ArrayList<GenericValue>(productPromoDiscountMapList.size());
>>>> -            Iterator<Map<Object, Object>>  
>>>> productPromoDiscountMapIter =  
>>>> productPromoDiscountMapList.iterator();
>>>> -            while (productPromoDiscountMapIter.hasNext()) {
>>>> -                Map<Object, Object> productPromoDiscountMap =  
>>>> UtilGenerics.checkMap(productPromoDiscountMapIter.next());
>>>> -                GenericValue productPromo = (GenericValue)  
>>>> productPromoDiscountMap.get("productPromo");
>>>> -                sortedProductPromoList.add(productPromo);
>>>> -                if (Debug.verboseOn()) Debug.logVerbose("Sorted  
>>>> Promo [" + productPromo.getString("productPromoId") + "] with  
>>>> total discount: " +  
>>>> productPromoDiscountMap.get("totalDiscountAmount"), module);
>>>> -            }
>>>> -
>>>>            // okay, all ready, do the real run, clearing the  
>>>> temporary result first...
>>>>            cart.clearAllPromotionInformation();
>>>> -            runProductPromos(sortedProductPromoList, cart,  
>>>> delegator, dispatcher, nowTimestamp, false);
>>>> +            runProductPromos(sortedExplodedProductPromoList,  
>>>> cart, delegator, dispatcher, nowTimestamp, false);
>>>>        } catch (NumberFormatException e) {
>>>>            Debug.logError(e, "Number not formatted correctly in  
>>>> promotion rules, not completed...", module);
>>>>        } catch (GenericEntityException e) {
>>>> @@ -436,7 +457,7 @@ public class ProductPromoWorker {
>>>>                                    GenericValue productPromoCode  
>>>> = productPromoCodeIter.next();
>>>>                                    String productPromoCodeId =  
>>>> productPromoCode.getString("productPromoCodeId");
>>>>                                    Long codeUseLimit =  
>>>> getProductPromoCodeUseLimit(productPromoCode, partyId, delegator);
>>>> -                                    if  
>>>> (runProductPromoRules(cart, cartChanged, useLimit, true,  
>>>> productPromoCodeId, codeUseLimit, maxUseLimit, productPromo,  
>>>> productPromoRules, dispatcher, delegator, nowTimestamp)) {
>>>> +                                    if  
>>>> (runProductPromoRules(cart, useLimit, true, productPromoCodeId,  
>>>> codeUseLimit, maxUseLimit, productPromo, productPromoRules,  
>>>> dispatcher, delegator, nowTimestamp)) {
>>>>                                        cartChanged = true;
>>>>                                    }
>>>>
>>>> @@ -448,7 +469,7 @@ public class ProductPromoWorker {
>>>>                            }
>>>>                        } else {
>>>>                            try {
>>>> -                                if (runProductPromoRules(cart,  
>>>> cartChanged, useLimit, false, null, null, maxUseLimit,  
>>>> productPromo, productPromoRules, dispatcher, delegator,  
>>>> nowTimestamp)) {
>>>> +                                if (runProductPromoRules(cart,  
>>>> useLimit, false, null, null, maxUseLimit, productPromo,  
>>>> productPromoRules, dispatcher, delegator, nowTimestamp)) {
>>>>                                    cartChanged = true;
>>>>                                }
>>>>                            } catch (RuntimeException e) {
>>>> @@ -735,8 +756,10 @@ public class ProductPromoWorker {
>>>>        return promoDescBuf.toString();
>>>>    }
>>>>
>>>> -    protected static boolean runProductPromoRules(ShoppingCart  
>>>> cart, boolean cartChanged, Long useLimit, boolean requireCode,  
>>>> String productPromoCodeId, Long codeUseLimit, long maxUseLimit,
>>>> +    protected static boolean runProductPromoRules(ShoppingCart  
>>>> cart, Long useLimit, boolean requireCode, String  
>>>> productPromoCodeId, Long codeUseLimit, long maxUseLimit,
>>>>        GenericValue productPromo, List<GenericValue>  
>>>> productPromoRules, LocalDispatcher dispatcher, Delegator  
>>>> delegator, Timestamp nowTimestamp) throws GenericEntityException,  
>>>> UseLimitException {
>>>> +        boolean cartChanged = false;
>>>> +        Map<ShoppingCartItem,BigDecimal> usageInfoMap =  
>>>> prepareProductUsageInfoMap(cart);
>>>>        String productPromoId = productPromo.getString("productPromoId");
>>>>        while ((useLimit == null || useLimit.longValue() >  
>>>> cart.getProductPromoUseCount(productPromoId)) &&
>>>>                (!requireCode ||  
>>>> UtilValidate.isNotEmpty(productPromoCodeId)) &&
>>>> @@ -755,17 +778,17 @@ public class ProductPromoWorker {
>>>>                // loop through conditions for rule, if any false,  
>>>> set allConditionsTrue to false
>>>>                List<GenericValue> productPromoConds =  
>>>> delegator.findByAnd("ProductPromoCond",  
>>>> UtilMisc.toMap("productPromoId",  
>>>> productPromo.get("productPromoId")),  
>>>> UtilMisc.toList("productPromoCondSeqId"), true);
>>>>                productPromoConds =  
>>>> EntityUtil.filterByAnd(productPromoConds,  
>>>> UtilMisc.toMap("productPromoRuleId",  
>>>> productPromoRule.get("productPromoRuleId")));
>>>> -                // using the other method to consolodate cache  
>>>> entries because the same cache is used elsewhere: List  
>>>> productPromoConds =  
>>>> productPromoRule.getRelated("ProductPromoCond", null,  
>>>> UtilMisc.toList("productPromoCondSeqId"), true);
>>>> +                // using the other method to consolidate cache  
>>>> entries because the same cache is used elsewhere: List  
>>>> productPromoConds =  
>>>> productPromoRule.getRelated("ProductPromoCond", null,  
>>>> UtilMisc.toList("productPromoCondSeqId"), true);
>>>>                if (Debug.verboseOn()) Debug.logVerbose("Checking  
>>>> " + productPromoConds.size() + " conditions for rule " +  
>>>> productPromoRule, module);
>>>>
>>>>                Iterator<GenericValue> productPromoCondIter =  
>>>> UtilMisc.toIterator(productPromoConds);
>>>>                while (productPromoCondIter != null &&  
>>>> productPromoCondIter.hasNext()) {
>>>>                    GenericValue productPromoCond =  
>>>> productPromoCondIter.next();
>>>>
>>>> -                    boolean condResult =  
>>>> checkCondition(productPromoCond, cart, delegator, dispatcher,  
>>>> nowTimestamp);
>>>> +                    boolean conditionSatisfied =  
>>>> checkCondition(productPromoCond, cart, delegator, dispatcher,  
>>>> nowTimestamp);
>>>>
>>>>                    // any false condition will cause it to NOT  
>>>> perform the action
>>>> -                    if (condResult == false) {
>>>> +                    if (!conditionSatisfied) {
>>>>                        performActions = false;
>>>>                        break;
>>>>                    }
>>>> @@ -797,13 +820,16 @@ public class ProductPromoWorker {
>>>>            }
>>>>
>>>>            if (promoUsed) {
>>>> -                
>>>> cart.addProductPromoUse(productPromo.getString("productPromoId"),  
>>>> productPromoCodeId, totalDiscountAmount, quantityLeftInActions);
>>>> +                // Get product use information from the cart
>>>> +                Map<ShoppingCartItem,BigDecimal> newUsageInfoMap  
>>>> = prepareProductUsageInfoMap(cart);
>>>> +                Map<ShoppingCartItem,BigDecimal>  
>>>> deltaUsageInfoMap = prepareDeltaProductUsageInfoMap(usageInfoMap,  
>>>> newUsageInfoMap);
>>>> +                usageInfoMap = newUsageInfoMap;
>>>> +                
>>>> cart.addProductPromoUse(productPromo.getString("productPromoId"),  
>>>> productPromoCodeId, totalDiscountAmount, quantityLeftInActions,  
>>>> deltaUsageInfoMap);
>>>>            } else {
>>>>                // the promotion was not used, don't try again  
>>>> until we finish a full pass and come back to see the promo  
>>>> conditions are now satisfied based on changes to the cart
>>>>                break;
>>>>            }
>>>>
>>>> -
>>>>            if (cart.getProductPromoUseCount(productPromoId) >  
>>>> maxUseLimit) {
>>>>                throw new UseLimitException("ERROR: While  
>>>> calculating promotions the promotion [" + productPromoId + "]  
>>>> action was applied more than " + maxUseLimit + " times, so the  
>>>> calculation has been ended. This should generally never happen  
>>>> unless you have bad rule definitions.");
>>>>            }
>>>> @@ -812,6 +838,34 @@ public class ProductPromoWorker {
>>>>        return cartChanged;
>>>>    }
>>>>
>>>> +    private static Map<ShoppingCartItem,BigDecimal>  
>>>> prepareProductUsageInfoMap(ShoppingCart cart) {
>>>> +        Map<ShoppingCartItem,BigDecimal> usageInfoMap = new  
>>>> HashMap<ShoppingCartItem, BigDecimal>();
>>>> +        List<ShoppingCartItem> lineOrderedByBasePriceList =  
>>>> cart.getLineListOrderedByBasePrice(false);
>>>> +        for (ShoppingCartItem cartItem : lineOrderedByBasePriceList) {
>>>> +            BigDecimal used = cartItem.getPromoQuantityUsed();
>>>> +            if (used.compareTo(BigDecimal.ZERO) != 0) {
>>>> +                usageInfoMap.put(cartItem, used);
>>>> +            }
>>>> +        }
>>>> +        return usageInfoMap;
>>>> +    }
>>>> +
>>>> +    private static Map<ShoppingCartItem,BigDecimal>  
>>>> prepareDeltaProductUsageInfoMap(Map<ShoppingCartItem,BigDecimal>  
>>>> oldMap, Map<ShoppingCartItem,BigDecimal> newMap) {
>>>> +        Map<ShoppingCartItem,BigDecimal> deltaUsageInfoMap = new  
>>>> HashMap<ShoppingCartItem, BigDecimal>(newMap);
>>>> +        Iterator<ShoppingCartItem> cartLines =  
>>>> oldMap.keySet().iterator();
>>>> +        while (cartLines.hasNext()) {
>>>> +            ShoppingCartItem cartLine = cartLines.next();
>>>> +            BigDecimal oldUsed = oldMap.get(cartLine);
>>>> +            BigDecimal newUsed = newMap.get(cartLine);
>>>> +            if (newUsed.compareTo(oldUsed) > 0) {
>>>> +                deltaUsageInfoMap.put(cartLine,  
>>>> newUsed.add(oldUsed.negate()));
>>>> +            } else {
>>>> +                deltaUsageInfoMap.remove(cartLine);
>>>> +            }
>>>> +        }
>>>> +        return deltaUsageInfoMap;
>>>> +    }
>>>> +
>>>>    protected static boolean checkCondition(GenericValue  
>>>> productPromoCond, ShoppingCart cart, Delegator delegator,  
>>>> LocalDispatcher dispatcher, Timestamp nowTimestamp) throws  
>>>> GenericEntityException {
>>>>        String condValue = productPromoCond.getString("condValue");
>>>>        String otherValue = productPromoCond.getString("otherValue");
>>>> @@ -1772,8 +1826,8 @@ public class ProductPromoWorker {
>>>>            actionResultInfo.ranAction = false;
>>>>        }
>>>>
>>>> +        // in action, if doesn't have enough quantity to use the  
>>>> promo at all, remove candidate promo uses and increment  
>>>> promoQuantityUsed; this should go for all actions, if any action  
>>>> runs we confirm
>>>>        if (actionResultInfo.ranAction) {
>>>> -            // in action, if doesn't have enough quantity to use  
>>>> the promo at all, remove candidate promo uses and increment  
>>>> promoQuantityUsed; this should go for all actions, if any action  
>>>> runs we confirm
>>>>            
>>>> cart.confirmPromoRuleUse(productPromoAction.getString("productPromoId"),  
>>>> productPromoAction.getString("productPromoRuleId"));
>>>>        } else {
>>>>            
>>>> cart.resetPromoRuleUse(productPromoAction.getString("productPromoId"),  
>>>> productPromoAction.getString("productPromoRuleId"));
>>>>
>>>>
>>>>
>>>
>>>
>>>
>>
>
>