From e44c025d7ef0e20f962eb5554add2fd5eb4d6a54 Mon Sep 17 00:00:00 2001 From: Jacopo Cappellato Date: Tue, 31 Dec 2013 07:44:19 +0000 Subject: [PATCH] Applied fix from trunk for revision: 1554265 === MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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) git-svn-id: https://svn.apache.org/repos/asf/ofbiz/branches/release13.07@1554381 13f79535-47bb-0310-9956-ffa450edef68 --- .../order/shoppingcart/ShoppingCart.java | 30 ++++- .../shoppingcart/ShoppingCartServices.java | 3 +- .../product/ProductPromoWorker.java | 124 +++++++++++++----- 3 files changed, 116 insertions(+), 41 deletions(-) diff --git a/applications/order/src/org/ofbiz/order/shoppingcart/ShoppingCart.java b/applications/order/src/org/ofbiz/order/shoppingcart/ShoppingCart.java index 7b5c7bf3720..58f978e115c 100644 --- a/applications/order/src/org/ofbiz/order/shoppingcart/ShoppingCart.java +++ b/applications/order/src/org/ofbiz/order/shoppingcart/ShoppingCart.java @@ -2714,7 +2714,7 @@ public BigDecimal getSubTotalForPromotions() { } itemsTotal = itemsTotal.add(cartItem.getItemSubTotal()); } - return itemsTotal; + return itemsTotal.add(this.getOrderOtherAdjustmentTotal()); } /** @@ -3142,12 +3142,12 @@ public Map getAllDesiredAlternateGiftByActionCopy() { return new HashMap(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 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 boolean equals(Object obj) { } } - public static class ProductPromoUseInfo implements Serializable { + public static class ProductPromoUseInfo implements Serializable, Comparable { public String productPromoId = null; public String productPromoCodeId = null; public BigDecimal totalDiscountAmount = BigDecimal.ZERO; public BigDecimal quantityLeftInActions = BigDecimal.ZERO; + private Map usageInfoMap = null; - public ProductPromoUseInfo(String productPromoId, String productPromoCodeId, BigDecimal totalDiscountAmount, BigDecimal quantityLeftInActions) { + public ProductPromoUseInfo(String productPromoId, String productPromoCodeId, BigDecimal totalDiscountAmount, BigDecimal quantityLeftInActions, Map 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 getUsageInfoMap() { return this.usageInfoMap; } + public BigDecimal getUsageWeight() { + Iterator 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 { diff --git a/applications/order/src/org/ofbiz/order/shoppingcart/ShoppingCartServices.java b/applications/order/src/org/ofbiz/order/shoppingcart/ShoppingCartServices.java index c3968a68be3..bf22edfecd7 100644 --- a/applications/order/src/org/ofbiz/order/shoppingcart/ShoppingCartServices.java +++ b/applications/order/src/org/ofbiz/order/shoppingcart/ShoppingCartServices.java @@ -21,6 +21,7 @@ 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 static Map assignItemShipGroup(DispatchContext dctx, Map< 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()); } } diff --git a/applications/order/src/org/ofbiz/order/shoppingcart/product/ProductPromoWorker.java b/applications/order/src/org/ofbiz/order/shoppingcart/product/ProductPromoWorker.java index b453dc627ae..6fa5a111b3c 100644 --- a/applications/order/src/org/ofbiz/order/shoppingcart/product/ProductPromoWorker.java +++ b/applications/order/src/org/ofbiz/order/shoppingcart/product/ProductPromoWorker.java @@ -22,6 +22,8 @@ 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.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 static void doPromotions(ShoppingCart cart, List productPro // 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 + // NOTE: we can easily recognize the promos for the order total: they are the ones with usage set to 0 + Iterator promoUses = cart.getProductPromoUseInfoIter(); + List sortedPromoUses = new ArrayList(); + while (promoUses.hasNext()) { + ProductPromoUseInfo promoUse = promoUses.next(); + sortedPromoUses.add(promoUse); + } + Collections.sort(sortedPromoUses); + List sortedExplodedProductPromoList = new ArrayList(sortedPromoUses.size()); + Map usesPerPromo = new HashMap(); + 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; + } - // 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> productPromoDiscountMapList = FastList.newInstance(); - List> productPromoDiscountMapListOrderTotal = FastList.newInstance(); for(GenericValue productPromo : productPromoList) { Map 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 sortedProductPromoList = new ArrayList(productPromoDiscountMapList.size()); - Iterator> productPromoDiscountMapIter = productPromoDiscountMapList.iterator(); - while (productPromoDiscountMapIter.hasNext()) { - Map 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 @@ protected static void runProductPromos(List productPromoList, Shop 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 @@ protected static void runProductPromos(List productPromoList, Shop } } 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 static String makeAutoDescription(GenericValue productPromo, Delegator de 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 productPromoRules, LocalDispatcher dispatcher, Delegator delegator, Timestamp nowTimestamp) throws GenericEntityException, UseLimitException { + boolean cartChanged = false; + Map usageInfoMap = prepareProductUsageInfoMap(cart); String productPromoId = productPromo.getString("productPromoId"); while ((useLimit == null || useLimit.longValue() > cart.getProductPromoUseCount(productPromoId)) && (!requireCode || UtilValidate.isNotEmpty(productPromoCodeId)) && @@ -755,17 +778,17 @@ protected static boolean runProductPromoRules(ShoppingCart cart, boolean cartCha // loop through conditions for rule, if any false, set allConditionsTrue to false List 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 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 @@ protected static boolean runProductPromoRules(ShoppingCart cart, boolean cartCha } if (promoUsed) { - cart.addProductPromoUse(productPromo.getString("productPromoId"), productPromoCodeId, totalDiscountAmount, quantityLeftInActions); + // Get product use information from the cart + Map newUsageInfoMap = prepareProductUsageInfoMap(cart); + Map 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 @@ protected static boolean runProductPromoRules(ShoppingCart cart, boolean cartCha return cartChanged; } + private static Map prepareProductUsageInfoMap(ShoppingCart cart) { + Map usageInfoMap = new HashMap(); + List 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 prepareDeltaProductUsageInfoMap(Map oldMap, Map newMap) { + Map deltaUsageInfoMap = new HashMap(newMap); + Iterator 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 static void performAction(ActionResultInfo actionResultInfo, GenericValue 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"));