From 51c7afff48871335a79af849d16bbaea15454e0e Mon Sep 17 00:00:00 2001 From: Adrian Torres Date: Mon, 12 Nov 2018 19:03:15 +0100 Subject: [PATCH 1/6] [FIX] sale_coupon: restore SO processing time This optimizes the confirmation of a sale_order in the ecommerce by ~73%, for a cart with 160 unique items, the processing time goes from 7m46s to 39s, which is roughly the same amount of processing time if the module `sale_coupon` is not installed. This optimization is achieved in two steps: 1. By validating promotional/discounted products in batch instead of validating each product one at a time. 2. By performing an unlink() on invalid reward lines instead of sending a delete command by writing to the one2many, as delete commands are not (yet) performed in batch, which means record recomputation after every delete. opw-1893277 --- models/sale_coupon_program.py | 14 +++++++++++--- models/sale_order.py | 4 ++-- 2 files changed, 13 insertions(+), 5 deletions(-) diff --git a/models/sale_coupon_program.py b/models/sale_coupon_program.py index df3687597a8..2a3168623c4 100644 --- a/models/sale_coupon_program.py +++ b/models/sale_coupon_program.py @@ -209,13 +209,14 @@ class SaleCouponProgram(models.Model): or Buy 1 coke + get 1 coke free then check 2 cokes are on cart or not """ order_lines = order.order_line - order._get_reward_lines() - products_qties = dict.fromkeys([product for product in order_lines.mapped('product_id')], 0) + products = order_lines.mapped('product_id') + products_qties = dict.fromkeys(products, 0) for line in order_lines: products_qties[line.product_id] += line.product_uom_qty valid_programs = self.filtered(lambda program: not program.rule_products_domain) for program in self - valid_programs: - ordered_rule_products_qty = sum(map(lambda product, qty: - program._is_valid_product(product) and qty, products_qties, products_qties.values())) + valid_products = program._get_valid_products(products) + ordered_rule_products_qty = sum(products_qties[product] for product in valid_products) # Avoid program if 1 ordered foo on a program '1 foo, 1 free foo' if program._is_valid_product(program.reward_product_id) and program.reward_type == 'product': line = order.order_line.filtered(lambda line: line.product_id == program.reward_product_id) @@ -266,8 +267,15 @@ class SaleCouponProgram(models.Model): return True def _is_valid_product(self, product): + # NOTE: if you override this method, think of also overriding _get_valid_products if self.rule_products_domain: domain = safe_eval(self.rule_products_domain) + [('id', '=', product.id)] return bool(self.env['product.product'].search_count(domain)) else: return True + + def _get_valid_products(self, products): + if self.rule_products_domain: + domain = safe_eval(self.rule_products_domain) + [('id', 'in', products.ids)] + return self.env['product.product'].search(domain) + return products diff --git a/models/sale_order.py b/models/sale_order.py index 1c0731650e1..5331fe1a157 100644 --- a/models/sale_order.py +++ b/models/sale_order.py @@ -347,8 +347,8 @@ class SaleOrder(models.Model): order.no_code_promo_program_ids -= programs_to_remove order.code_promo_program_id -= programs_to_remove order.applied_coupon_ids -= order.applied_coupon_ids.filtered(lambda coupon: coupon.program_id in programs_to_remove) - invalid_lines += order.order_line.filtered(lambda line: line.product_id.id in products_to_remove.ids) - order.write({'order_line': [(2, line.id, False) for line in invalid_lines]}) + invalid_lines |= order.order_line.filtered(lambda line: line.product_id.id in products_to_remove.ids) + invalid_lines.unlink() def _get_applied_programs_with_rewards_on_current_order(self): # Need to add filter on current order. Indeed, it has always been calculating reward line even if on next order (which is useless and do calculation for nothing) From 7b99c0db92e42b712104abc7147eb9ed807ceb2f Mon Sep 17 00:00:00 2001 From: Romain Derie Date: Fri, 9 Nov 2018 14:13:01 +0100 Subject: [PATCH 2/6] [FIX] sale_coupon: generate only one coupon per program per SO Before this commit: Everytime the SO met a 'on_next_order' program requirements, a coupon would be generated on that SO. Even if there was already a coupon for that program set to 'expired', for instance when the SO did not met the requirements anymore. Now: When the requirements are met, it checks that an `expired` coupon does not already exist. It it does, it will set that coupon back to `reserved` state. Step to reproduce: - Create a program on `next_order` with requirements, eg $300 amount - Create a SO and add $300 worth of products - Update promotion, a coupon is created and set to `reserved` - Now update the SO to go below $300 - Update promotion, the coupon is set to `expired` as it should - Update again the SO to meet the $300 requirements again - Update promotion, a new `reserved` coupon is generated instead of updating the state of the previous one from `expired` to `reserved` Closes #3062 --- models/sale_order.py | 25 ++++++++++++++++++------- 1 file changed, 18 insertions(+), 7 deletions(-) diff --git a/models/sale_order.py b/models/sale_order.py index 5331fe1a157..3324dce2d2a 100644 --- a/models/sale_order.py +++ b/models/sale_order.py @@ -216,13 +216,24 @@ class SaleOrder(models.Model): self.write({'order_line': [(0, False, value) for value in self._get_reward_line_values(program)]}) def _create_reward_coupon(self, program): - coupon = self.env['sale.coupon'].create({ - 'program_id': program.id, - 'state': 'reserved', - 'partner_id': self.partner_id.id, - 'order_id': self.id, - 'discount_line_product_id': program.discount_line_product_id.id - }) + # if there is already a coupon that was set as expired, reactivate that one instead of creating a new one + coupon = self.env['sale.coupon'].search([ + ('program_id', '=', program.id), + ('state', '=', 'expired'), + ('partner_id', '=', self.partner_id.id), + ('order_id', '=', self.id), + ('discount_line_product_id', '=', program.discount_line_product_id.id), + ], limit=1) + if coupon: + coupon.write({'state': 'reserved'}) + else: + coupon = self.env['sale.coupon'].create({ + 'program_id': program.id, + 'state': 'reserved', + 'partner_id': self.partner_id.id, + 'order_id': self.id, + 'discount_line_product_id': program.discount_line_product_id.id + }) self.generated_coupon_ids |= coupon return coupon From e8efcb1bc34353ad89bf027d28eee6977de8cd13 Mon Sep 17 00:00:00 2001 From: Romain Derie Date: Fri, 9 Nov 2018 14:23:49 +0100 Subject: [PATCH 3/6] [FIX] sale_coupon: do not deduce reward quantity on current_order Before this commit: 1. We were checking if the rewards were in the SO even if it was 'next_order' program. 2. We were deducing the rewards quantity for 'next_order' program when checking if we had enough products to meet the program requirements. Now: 1. We don't verify if the reward is on the SO if the program is 'next_order' 2. We don't deduce reward quantity when checking if enough products to meet the program requirements if the program is 'next_order' opw-1907211 Closes #3062 --- models/sale_coupon_program.py | 12 ++++++++---- tests/test_program_numbers.py | 28 ++++++++++++++++++++++++++++ 2 files changed, 36 insertions(+), 4 deletions(-) diff --git a/models/sale_coupon_program.py b/models/sale_coupon_program.py index 2a3168623c4..820310a41a8 100644 --- a/models/sale_coupon_program.py +++ b/models/sale_coupon_program.py @@ -218,10 +218,9 @@ class SaleCouponProgram(models.Model): valid_products = program._get_valid_products(products) ordered_rule_products_qty = sum(products_qties[product] for product in valid_products) # Avoid program if 1 ordered foo on a program '1 foo, 1 free foo' - if program._is_valid_product(program.reward_product_id) and program.reward_type == 'product': - line = order.order_line.filtered(lambda line: line.product_id == program.reward_product_id) + if program.promo_applicability == 'on_current_order' and \ + program._is_valid_product(program.reward_product_id) and program.reward_type == 'product': ordered_rule_products_qty -= program.reward_product_quantity - # needed_quantity = program.rule_min_quantity if self. if ordered_rule_products_qty >= program.rule_min_quantity: valid_programs |= program return valid_programs @@ -256,7 +255,12 @@ class SaleCouponProgram(models.Model): # Product requirement should not be checked if the coupon got generated by a promotion program (the requirement should have only be checked to generate the coupon) if not next_order: programs = programs and programs._filter_programs_on_products(order) - programs = programs and programs._filter_not_ordered_reward_programs(order) + + programs_curr_order = programs.filtered(lambda p: p.promo_applicability == 'on_current_order') + programs = programs.filtered(lambda p: p.promo_applicability == 'on_next_order') + if programs_curr_order: + # Checking if rewards are in the SO should not be performed for rewards on_next_order + programs += programs_curr_order._filter_not_ordered_reward_programs(order) return programs def _is_valid_partner(self, partner): diff --git a/tests/test_program_numbers.py b/tests/test_program_numbers.py index 8284cd1dad4..024781da232 100644 --- a/tests/test_program_numbers.py +++ b/tests/test_program_numbers.py @@ -489,3 +489,31 @@ class TestSaleCouponProgramNumbers(TestSaleCouponCommon): fixed_amount_program.write({'active': False}) # Check archived product will remove discount lines on recompute order.recompute_coupon_lines() self.assertEqual(len(order.order_line.ids), 1, "Archiving the program should remove the program reward line") + + def test_program_next_order(self): + order = self.empty_order + self.env['sale.coupon.program'].create({ + 'name': 'Free Keyboard if at least 1 article', + 'promo_code_usage': 'no_code_needed', + 'promo_applicability': 'on_next_order', + 'program_type': 'promotion_program', + 'reward_type': 'product', + 'reward_product_id': self.wirelessKeyboard.id, + 'rule_min_quantity': 2, + }) + sol1 = self.env['sale.order.line'].create({ + 'product_id': self.iPadMini.id, + 'name': 'iPad Mini', + 'product_uom_qty': 1.0, + 'order_id': order.id, + }) + order.recompute_coupon_lines() + self.assertEqual(len(order.order_line.ids), 1, "Nothing should be added to the cart") + self.assertEqual(len(order.generated_coupon_ids), 0, "No coupon should have been generated yet") + + sol1.product_uom_qty = 2 + order.recompute_coupon_lines() + generated_coupon = order.generated_coupon_ids + self.assertEqual(len(order.order_line.ids), 1, "Nothing should be added to the cart (2)") + self.assertEqual(len(generated_coupon), 1, "A coupon should have been generated") + self.assertEqual(generated_coupon.state, 'reserved', "The coupon should be reserved") From 5a75d1aeae77c6bfb3ee1ba86115e46777b76bbf Mon Sep 17 00:00:00 2001 From: Romain Derie Date: Fri, 9 Nov 2018 14:44:28 +0100 Subject: [PATCH 4/6] [FIX] sale_coupon: unvalid next_order programs if needed Before this commit: We would never unvalid a `next_order` program (and thus it's generated coupon) Now: We also retrieve applied `next_order` programs as we need to check if they are still valid too Step to reproduce: - Create a program on `next_order` that require $300 - Add $300 worth of product on a SO - Apply program, it will generate a coupon - Make the SO total go below $300 - The generated coupon is still Valid and the client will receive that coupon even if it is not supposed to anymore Closes #3062 --- models/sale_order.py | 6 +++++- tests/test_program_numbers.py | 13 +++++++++++++ 2 files changed, 18 insertions(+), 1 deletion(-) diff --git a/models/sale_order.py b/models/sale_order.py index 3324dce2d2a..c814ce5be24 100644 --- a/models/sale_order.py +++ b/models/sale_order.py @@ -347,7 +347,7 @@ class SaleOrder(models.Model): order = self applicable_programs = order._get_applicable_no_code_promo_program() + order._get_applicable_programs() + order._get_applied_coupon_program_coming_from_another_so() - applied_programs = order._get_applied_programs_with_rewards_on_current_order() + applied_programs = order._get_applied_programs_with_rewards_on_current_order() + order._get_applied_programs_with_rewards_on_next_order() programs_to_remove = applied_programs - applicable_programs products_to_remove = programs_to_remove.mapped('discount_line_product_id') @@ -370,6 +370,10 @@ class SaleOrder(models.Model): self.applied_coupon_ids.mapped('program_id') + \ self.code_promo_program_id.filtered(lambda p: p.promo_applicability == 'on_current_order') + def _get_applied_programs_with_rewards_on_next_order(self): + return self.no_code_promo_program_ids.filtered(lambda p: p.promo_applicability == 'on_next_order') + \ + self.code_promo_program_id.filtered(lambda p: p.promo_applicability == 'on_next_order') + class SaleOrderLine(models.Model): _inherit = "sale.order.line" diff --git a/tests/test_program_numbers.py b/tests/test_program_numbers.py index 024781da232..7869c3b80a6 100644 --- a/tests/test_program_numbers.py +++ b/tests/test_program_numbers.py @@ -517,3 +517,16 @@ class TestSaleCouponProgramNumbers(TestSaleCouponCommon): self.assertEqual(len(order.order_line.ids), 1, "Nothing should be added to the cart (2)") self.assertEqual(len(generated_coupon), 1, "A coupon should have been generated") self.assertEqual(generated_coupon.state, 'reserved', "The coupon should be reserved") + + sol1.product_uom_qty = 1 + order.recompute_coupon_lines() + generated_coupon = order.generated_coupon_ids + self.assertEqual(len(order.order_line.ids), 1, "Nothing should be added to the cart (3)") + self.assertEqual(len(generated_coupon), 1, "No more coupon should have been generated and the existing one should not have been deleted") + self.assertEqual(generated_coupon.state, 'expired', "The coupon should have been set as expired as it is no more valid since we don't have the required quantity") + + sol1.product_uom_qty = 2 + order.recompute_coupon_lines() + generated_coupon = order.generated_coupon_ids + self.assertEqual(len(generated_coupon), 1, "We should still have only 1 coupon as we now benefit again from the program but no need to create a new one (see next assert)") + self.assertEqual(generated_coupon.state, 'reserved', "The coupon should be set back to reserved as we had already an expired one, no need to create a new one") From 231669e33269257d1539c9a1f13189a332dda668 Mon Sep 17 00:00:00 2001 From: Romain Derie Date: Mon, 12 Nov 2018 15:24:24 +0100 Subject: [PATCH 5/6] [FIX] sale_coupon: remove coupon_program reward if req. not met anymore Before this commit: If a reward had been given on the SO with a coupon_program code and that the program had requirements and that these requirements are not met anymore, recomputing the SO would not remove the reward. Now: If the SO does not met the program requirements anymore, we remove the reward Step to reproduce: - Create a coupon_program with quantity or amount requirements (eg: $200) - Generate a coupon for that program - Add $200 worth of products (matching the program domain) in the SO - Add the generated coupon to that SO - You should get the reward as you have $200 in the SO - Remove some products from the SO to go below $200 - Recompute coupons, the reward won't be removed Closes #3062 --- models/sale_order.py | 13 +++++-- tests/test_program_rules.py | 67 +++++++++++++++++++++++++++++++++++++ 2 files changed, 78 insertions(+), 2 deletions(-) diff --git a/models/sale_order.py b/models/sale_order.py index c814ce5be24..e043d691f60 100644 --- a/models/sale_order.py +++ b/models/sale_order.py @@ -270,8 +270,17 @@ class SaleOrder(models.Model): return programs def _get_applied_coupon_program_coming_from_another_so(self): + # TODO: Remove me in master as no more used + pass + + def _get_valid_applied_coupon_program(self): self.ensure_one() - programs = self.applied_coupon_ids.mapped('program_id')._filter_programs_from_common_rules(self, True) + # applied_coupon_ids's coupons might be coming from: + # * a coupon generated from a previous order that benefited from a promotion_program that rewarded the next sale order. + # In that case requirements to benefit from the program (Quantity and price) should not be checked anymore + # * a coupon_program, in that case the promo_applicability is always for the current order and everything should be checked (filtered) + programs = self.applied_coupon_ids.mapped('program_id').filtered(lambda p: p.promo_applicability == 'on_next_order')._filter_programs_from_common_rules(self, True) + programs += self.applied_coupon_ids.mapped('program_id').filtered(lambda p: p.promo_applicability == 'on_current_order')._filter_programs_from_common_rules(self) return programs def _create_new_no_code_promo_reward_lines(self): @@ -346,7 +355,7 @@ class SaleOrder(models.Model): self.ensure_one() order = self - applicable_programs = order._get_applicable_no_code_promo_program() + order._get_applicable_programs() + order._get_applied_coupon_program_coming_from_another_so() + applicable_programs = order._get_applicable_no_code_promo_program() + order._get_applicable_programs() + order._get_valid_applied_coupon_program() applied_programs = order._get_applied_programs_with_rewards_on_current_order() + order._get_applied_programs_with_rewards_on_next_order() programs_to_remove = applied_programs - applicable_programs products_to_remove = programs_to_remove.mapped('discount_line_product_id') diff --git a/tests/test_program_rules.py b/tests/test_program_rules.py index 181b5adb07e..5339e00cb05 100644 --- a/tests/test_program_rules.py +++ b/tests/test_program_rules.py @@ -4,6 +4,7 @@ from datetime import datetime, timedelta from openerp.addons.sale_coupon.tests.common import TestSaleCouponCommon +from odoo.exceptions import UserError from odoo.fields import Date class TestProgramRules(TestSaleCouponCommon): @@ -171,3 +172,69 @@ class TestProgramRules(TestSaleCouponCommon): ]}) order.recompute_coupon_lines() self.assertEqual(len(order.order_line.ids), 2, "The promo offert shouldn't have been applied as the number of uses is exceeded") + + def test_program_rules_coupon_qty_and_amount_remove_not_eligible(self): + ''' This test will: + * Check quantity and amount requirements works as expected (since it's slightly different from a promotion_program) + * Ensure that if a reward from a coupon_program was allowed and the conditions are not met anymore, + the reward will be removed on recompute. + ''' + self.immediate_promotion_program.active = False # Avoid having this program to add rewards on this test + order = self.empty_order + + program = self.env['sale.coupon.program'].create({ + 'name': 'Get 10% discount if buy at least 4 Product A and $320', + 'program_type': 'coupon_program', + 'reward_type': 'discount', + 'discount_type': 'percentage', + 'discount_percentage': 10.0, + 'rule_products_domain': "[('id', 'in', [%s])]" % (self.product_A.id), + 'rule_min_quantity': 3, + 'rule_minimum_amount': 320.00, + }) + + sol1 = self.env['sale.order.line'].create({ + 'product_id': self.product_A.id, + 'name': 'Product A', + 'product_uom_qty': 2.0, + 'order_id': order.id, + }) + + sol2 = self.env['sale.order.line'].create({ + 'product_id': self.product_B.id, + 'name': 'Product B', + 'product_uom_qty': 4.0, + 'order_id': order.id, + }) + + # Default value for coupon generate wizard is generate by quantity and generate only one coupon + self.env['sale.coupon.generate'].with_context(active_id=program.id).create({}).generate_coupon() + coupon = program.coupon_ids[0] + + # Not enough amount since we only have 220 (100*2 + 5*4) + with self.assertRaises(UserError): + self.env['sale.coupon.apply.code'].with_context(active_id=order.id).create({ + 'coupon_code': coupon.code + }).process_coupon() + + sol2.product_uom_qty = 24 + + # Not enough qty since we only have 3 Product A (Amount is ok: 100*2 + 5*24 = 320) + with self.assertRaises(UserError): + self.env['sale.coupon.apply.code'].with_context(active_id=order.id).create({ + 'coupon_code': coupon.code + }).process_coupon() + + sol1.product_uom_qty = 3 + + self.env['sale.coupon.apply.code'].with_context(active_id=order.id).create({ + 'coupon_code': coupon.code + }).process_coupon() + order.recompute_coupon_lines() + + self.assertEqual(len(order.order_line.ids), 3, "The order should contains the Product A line, the Product B line and the discount line") + + sol1.product_uom_qty = 2 + order.recompute_coupon_lines() + + self.assertEqual(len(order.order_line.ids), 2, "The discount line should have been removed as we don't meet the program requirements") From 63131c258486002831cfbfa980214233fad201f9 Mon Sep 17 00:00:00 2001 From: Romain Derie Date: Mon, 12 Nov 2018 18:06:57 +0100 Subject: [PATCH 6/6] [FIX] sale_coupon: reset no more eligible applied coupon to Valid state Before this commit: When a coupon_program's coupon was giving a reward on a SO and then the recompute was removing that reward (eg the SO did not met the requirements anymore) the coupon would not be reset and would be lost. Now: If a coupon's reward got removed for some reason, the coupon got reset to a valid state (`new`) so it can be used again. Step to reproduce: - Generate a coupon from a coupon program that needs requirements (eg $300) - Add that coupon to a SO that has a total over $300 - Coupon is set as `used` and the reward is granted - Remove some products to set the SO total below $300 - Recompute coupon, the reward is correctly removed but the coupon is still set as `used` so it won't be usable again and is basically lost. Closes #3062 --- models/sale_order.py | 10 +++++++++- tests/test_program_rules.py | 2 ++ 2 files changed, 11 insertions(+), 1 deletion(-) diff --git a/models/sale_order.py b/models/sale_order.py index e043d691f60..d031c8543cd 100644 --- a/models/sale_order.py +++ b/models/sale_order.py @@ -363,10 +363,18 @@ class SaleOrder(models.Model): # delete reward line coming from an archived coupon (it will never be updated/removed when recomputing the order) invalid_lines = order.order_line.filtered(lambda line: line.is_reward_line and line.product_id.id not in (applied_programs).mapped('discount_line_product_id').ids) + # Invalid generated coupon for which we are not eligible anymore ('expired' since it is specific to this SO and we may again met the requirements) self.generated_coupon_ids.filtered(lambda coupon: coupon.program_id.discount_line_product_id.id in products_to_remove.ids).write({'state': 'expired'}) + # Reset applied coupons for which we are not eligible anymore ('valid' so it can be use on another ) + coupons_to_remove = order.applied_coupon_ids.filtered(lambda coupon: coupon.program_id in programs_to_remove) + coupons_to_remove.write({'state': 'new'}) + + # Unbind promotion and coupon programs which requirements are not met anymore order.no_code_promo_program_ids -= programs_to_remove order.code_promo_program_id -= programs_to_remove - order.applied_coupon_ids -= order.applied_coupon_ids.filtered(lambda coupon: coupon.program_id in programs_to_remove) + order.applied_coupon_ids -= coupons_to_remove + + # Remove their reward lines invalid_lines |= order.order_line.filtered(lambda line: line.product_id.id in products_to_remove.ids) invalid_lines.unlink() diff --git a/tests/test_program_rules.py b/tests/test_program_rules.py index 5339e00cb05..727f71bcadc 100644 --- a/tests/test_program_rules.py +++ b/tests/test_program_rules.py @@ -233,8 +233,10 @@ class TestProgramRules(TestSaleCouponCommon): order.recompute_coupon_lines() self.assertEqual(len(order.order_line.ids), 3, "The order should contains the Product A line, the Product B line and the discount line") + self.assertEqual(coupon.state, 'used', "The coupon should be set to Consumed as it has been used") sol1.product_uom_qty = 2 order.recompute_coupon_lines() self.assertEqual(len(order.order_line.ids), 2, "The discount line should have been removed as we don't meet the program requirements") + self.assertEqual(coupon.state, 'new', "The coupon should be reset to Valid as it's reward got removed")