From 194b83eeea6a1d0846fb7809ead3bfee617381fd Mon Sep 17 00:00:00 2001 From: Romain Derie Date: Tue, 24 Apr 2018 16:09:49 +0200 Subject: [PATCH] [FIX] sale_coupon(_delivery): fix some flows (mostly on next order) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This commit is the continuity of a42c27f9 that fixed some bugs. One of the bug was that a discount was only applied on price without taxes. E.G: A $100 product with 20% excluded tax would cost $120 but a 50% discount would give the user only $50 discount and not $60 as expected. User would lose $10 discount. With these fixes, the discount line would correctly be set to $60 but since the discount line would not have tax, the merchant would lose the discount amount on the tax, in this case 50% of $20. This commit will improve this case by adding a tax on a discount line. Thus, since a global discount can be applied to multiple products having different taxes or even multiple taxes on the same product, a discount line should be split for every product taxes the discount applies on. Obviously, this is a big behavior change, even if the discount is now finally correct: the discount is applied on the total price and the merchant does not lose any taxes. To make it more clear, the discount lines description will mention on which taxes it is applied. There won't be any changes in the e-commerce since we will group the discount lines so it will appear as before even if there are multiple lines. This is only possible because we don't show taxes on line on the e-commerce. This commit also fixes more complex flows/use cases. Note: Fixes & flows will be illustrated with a SO but the behavior is the same with a cart on front-end. == Fixes == =========================================================================== The biggest flow that needed to be fixed is when a promotion program give a reward on the next order. Let's illustrate with a promotion program giving a free `product_B` on next order (so it will generate a coupon) if the SO meets the requirements. The requirements are: buy a `product_A` and buy for at least $600. (This program needs a code: 'free_b_on_next') 1. If the SO has a `product_A` and over $600, applying the promotion code `free_b_on_next` will error saying that `product_B` is not in the cart. This error should not appear, since the `product_B` is the reward for the next sale order that will use the generated coupon. Anyway, if you add `product_B` and apply the promotion code `free_b_on_next` to generate the coupon to check the rest of the flow. It will generate the coupon and send email to the customer. 2. Every time you will apply the promotion code `free_b_on_next`, it will regenerate a new coupon and resend a new email to the client. Then, create a second SO, add 2 `product_B` and try to apply the generated coupon from the first SO. 3. It will error saying that the $600 requirement is not met. It should not since the requirement was only required to generate the coupon on the first SO. User has now a coupon to enjoy a free `product_B` and should not buy $600. Anyway, let's add some products to meet the 600$ requirement to check the rest of the flow. 4. It will now error saying that `product_A` is not in the cart. As bug 3., it should not since this requirement was also required to generate the coupon on the first SO. Anyway, let's add `product_A` to meet the requirement to check the rest of the flow. 5. Updating the promotions will remove the reward and you won't be able to reapply the coupon since it was `used`. This bug was even more critical in front-end since any action on the cart will recompute the coupons and automatically delete the reward after adding it. The customer never saw the reward on his cart. This bug is related to 3. and 4. Indeed, when recomputing coupons, it will remove programs that are not applicable anymore. The code wrongly think it is the case with this coupon since we don't have $600 or a `product_A`. `_remove_invalid_reward_lines` should treat coupon coming from a promotion program set on `next_order` differently and not filter on requirements. =========================================================================== 6. (extension of 2.) Every time an user would meet the requirements of a promotion program generating a coupon for a next order, it would generate a coupon and send an email. E.g: If buy 2 iPads get 1 for free on next order. If user add and remove iPad for some reason, every time he has 2 iPads in his cart it will receive an email with a new coupon. 6'. When applying a promotion code that will reward next order, it will not be set in the SO `code_promo_program_id` (I did not change that behavior since it could make sense to be able to apply another coupon that will give -20% on current order). This will allow applying multiple promotion rewarding a next order and/or a promotion rewarding current order. (Which would not be possible if was stored on `code_promo_program_id` since you can only apply one code on an order) But this cause a bug: since the promotion program is not stored on the order, every time you will apply the coupon it will generate a new coupon and send an email. So if you work on a draft sale order and do some tests, it will send many emails to customer.. 7. Having multiple global discounts (on the whole SO) was forbidden: you can't apply a coupon promotion or a program promotion if there is already a global coupon discount or a global promotion discount with code. But still you could apply a global discount if there were a global promotion discount automatically applied (without a code). 8. On the generate coupon wizard, if you select `Number of Selected Customers`, by default it is set to 'Match all records (X records)'. But when you click on generate, it won't generate anything even if visually it selected every partner by default. This is because the domain default value is an empty string. Thus, the domain widget is selecting everything but once it is sent back to the server, since it's an empty string it won't get through the `if` condition. It was even weirder when you add something to the domain and remove it, you then visually come back to where you were at the beginning but the domain got changed from '' to '[]' so in this case it will generate the coupon for every partner. == More like improvements == 1. `action_cancel` and `action_confirm` would handle correctly the states of the applied and generated coupons (valid, consumed, expired..). But `action_draft` would not handle the states at all. 2. The generated coupon on a SO are now sent by email only when the SO is validated. This is part of the fix n°2 on coupon being generated multiple times and sent by email multiple times. The coupon sent by email was not usable anyway since it would error saying the SO that generated the coupon was not validated. 3. Discount line limited to a max amount (set in the program) will mention on the line description that the discount is limited. This will avoid cases where you don't understand why 50% discount of $2000 SO shows $40. 4. If you archive a program and there is still some SO containing this program's reward line, these line will be 'floating free' and will never be updated or removed. Indeed, the `_remove_invalid_reward_lines` is basically checking which programs could be applied on the order and which one are applied on the order. If a program is applied on the order but is not supposed to, his reward line will be removed. Of course, since the program got archived, it won't get retrieve and the line will never get removed. Even if the case of a free iPad: if you buy an iPad and you removed the paid iPad from the SO. The free iPad will remain forever. Note: We can't remove these discount line directly when archiving a program because there might be quotations that we don't want to modify since it is waiting for customer confirmation. Improved behavior: - Archiving a program archives its related reward product but unarchiving the program does not unarchived the product. task-1866977 opw-1832967 opw-1857843 Closes #2224 Comes with https://github.com/odoo/odoo/pull/24971 --- models/sale_order.py | 10 +++++----- tests/test_free_shipping_reward.py | 7 +++++-- 2 files changed, 10 insertions(+), 7 deletions(-) diff --git a/models/sale_order.py b/models/sale_order.py index e0f91648823..0d11cd8c159 100644 --- a/models/sale_order.py +++ b/models/sale_order.py @@ -13,14 +13,14 @@ class SaleOrder(models.Model): lines = self.order_line.filtered(lambda line: line.is_delivery or line.product_id in free_delivery_product) return lines + super(SaleOrder, self)._get_no_effect_on_threshold_lines() - def _get_order_lines_tax_included_amount(self): + def _get_paid_order_lines(self): """ Returns the taxes included sale order total amount without the rewards amount""" free_reward_product = self.env['sale.coupon.program'].search([('reward_type', '=', 'product')]).mapped('discount_line_product_id') - return sum([x.price_total for x in self.order_line.filtered(lambda x: not (x.is_reward_line or x.is_delivery) or x.product_id in free_reward_product)]) + return self.order_line.filtered(lambda x: not (x.is_reward_line or x.is_delivery) or x.product_id in free_reward_product) def _get_reward_line_values(self, program): if program.reward_type == 'free_shipping': - return self._get_reward_values_free_shipping(program) + return [self._get_reward_values_free_shipping(program)] else: return super(SaleOrder, self)._get_reward_line_values(program) @@ -40,9 +40,9 @@ class SaleOrder(models.Model): 'tax_id': [(4, tax.id, False) for tax in taxes], } - def _get_lines_unit_prices(self): + def _get_cheapest_line(self): # Unit prices tax included - return [x.price_total / x.product_uom_qty for x in self.order_line.filtered(lambda x: not x.is_delivery and not x.is_reward_line)] + return min(self.order_line.filtered(lambda x: not x.is_reward_line and not x.is_delivery and x.price_unit > 0), key=lambda x: x['price_unit']) class SalesOrderLine(models.Model): _inherit = "sale.order.line" diff --git a/tests/test_free_shipping_reward.py b/tests/test_free_shipping_reward.py index 5c8cce3d1a4..175a06d0c76 100644 --- a/tests/test_free_shipping_reward.py +++ b/tests/test_free_shipping_reward.py @@ -120,6 +120,9 @@ class TestSaleCouponProgramRules(TestSaleCouponCommon): self.assertEqual(len(order.order_line.ids), 3, "We should get the delivery line but not the free delivery since we are below 872.73$ with the 10% discount") p_minimum_threshold_free_delivery.sequence = 10 + (order.order_line - sol1).unlink() + order.carrier_id = self.env['delivery.carrier'].search([])[1] + order.delivery_set() order.recompute_coupon_lines() self.assertEqual(len(order.order_line.ids), 4, "We should get both promotion line since the free delivery will be applied first and won't change the SO total") @@ -193,5 +196,5 @@ class TestSaleCouponProgramRules(TestSaleCouponCommon): }) p_specific_product.discount_apply_on = 'cheapest_product' order.recompute_coupon_lines() - # 872.73 - (20% of 1 iPad) = 872.73 - 64 = 808.73 - self.assertEqual(order.amount_untaxed, 808.73, "One iPad should be discounted by 20%") + # 872.73 - (20% of 1 iPad) = 872.73 - 58.18 = 814.55 + self.assertEqual(order.amount_untaxed, 814.55, "One iPad should be discounted by 20%")