-
-
Notifications
You must be signed in to change notification settings - Fork 1.2k
[16.0][FIX] sale_discount_display_amount: fix onchange peformance #3706
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: 16.0
Are you sure you want to change the base?
Changes from all commits
b5f4346
27517b2
f1c95ae
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -2,53 +2,87 @@ | |
| # License AGPL-3.0 or later (http://www.gnu.org/licenses/agpl). | ||
|
|
||
| from odoo import api, fields, models | ||
| from odoo.tools import float_compare | ||
|
|
||
|
|
||
| class SaleOrderLine(models.Model): | ||
|
|
||
| _inherit = "sale.order.line" | ||
|
|
||
| discount_total = fields.Monetary( | ||
| compute="_compute_amount", | ||
| compute="_compute_discount_total", | ||
| string="Discount Subtotal", | ||
| store=True, | ||
| precompute=True, | ||
| ) | ||
| price_total_no_discount = fields.Monetary( | ||
| compute="_compute_amount", | ||
| compute="_compute_discount_total", | ||
| string="Subtotal Without Discount", | ||
| store=True, | ||
| precompute=True, | ||
| ) | ||
|
|
||
| # This hook method determines if a discount is applied to the sale order line. | ||
| # It provides an extension point (a "hook") for other modules to easily | ||
| # modify the behavior of the discount calculation without directly altering | ||
| # this module's code. For example, if a module like "sale_triple_discount" | ||
| # introduces more complex discount logic (e.g., global discounts, tiered discounts), | ||
| # it can override this method to correctly reflect whether a discount applies, | ||
| # thereby influencing the 'discount_total' and 'price_total_no_discount' computations. | ||
| def _has_discount(self): | ||
| self.ensure_one() | ||
| currency = self.currency_id or self.env.company.currency_id | ||
| return not currency.is_zero(self.discount) | ||
|
|
||
| def _update_discount_display_fields(self): | ||
| for line in self: | ||
| line.price_total_no_discount = 0 | ||
| line.discount_total = 0 | ||
| if not line.discount: | ||
| line.price_total_no_discount = line.price_total | ||
| continue | ||
| price = line.price_unit | ||
| taxes = line.tax_id.compute_all( | ||
| price, | ||
| line.order_id.currency_id, | ||
| line.product_uom_qty, | ||
| product=line.product_id, | ||
| partner=line.order_id.partner_shipping_id, | ||
| ) | ||
|
|
||
| price_total_no_discount = taxes["total_included"] | ||
| discount_total = price_total_no_discount - line.price_total | ||
|
|
||
| line.update( | ||
| { | ||
| "discount_total": discount_total, | ||
| "price_total_no_discount": price_total_no_discount, | ||
| } | ||
| ) | ||
|
|
||
| @api.depends("product_uom_qty", "discount", "price_unit", "tax_id") | ||
| def _compute_amount(self): | ||
| res = super(SaleOrderLine, self)._compute_amount() | ||
| price_total_no_discount = 0.0 | ||
| discount_total = 0.0 | ||
| currency = line.order_id.currency_id or self.env.company.currency_id | ||
| if not line._has_discount(): | ||
| price_total_no_discount = line.price_total | ||
| else: | ||
| price = line.price_unit | ||
| taxes = line.tax_id.compute_all( | ||
| price, | ||
| currency, | ||
| line.product_uom_qty, | ||
| product=line.product_id, | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The |
||
| partner=line.order_id.partner_shipping_id, | ||
| ) | ||
|
|
||
| price_total_no_discount = taxes["total_included"] | ||
| discount_total = price_total_no_discount - line.price_total | ||
| if ( | ||
| float_compare( | ||
| line.discount_total, | ||
| discount_total, | ||
| precision_rounding=currency.rounding, | ||
| ) | ||
| != 0 | ||
| ): | ||
| line.discount_total = discount_total | ||
| if ( | ||
| float_compare( | ||
| line.price_total_no_discount, | ||
| price_total_no_discount, | ||
| precision_rounding=currency.rounding, | ||
| ) | ||
| != 0 | ||
| ): | ||
| line.price_total_no_discount = price_total_no_discount | ||
|
|
||
| @api.depends( | ||
| "discount", | ||
| "price_total", | ||
| "product_uom_qty", | ||
| "product_id", | ||
| "tax_id", | ||
| "price_unit", | ||
| "order_id.currency_id", | ||
| "order_id.partner_shipping_id", | ||
| ) | ||
| def _compute_discount_total(self): | ||
| # we should move the logic from the _update_discount_display_fields | ||
| # to the _compute_discount_total method but to ensure the backward | ||
| # compatibility we keep the logic in both methods TODO in V19 | ||
| self._update_discount_display_fields() | ||
| return res | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,39 +1,60 @@ | ||
| # Copyright 2018 ACSONE SA/NV | ||
| # License AGPL-3.0 or later (http://www.gnu.org/licenses/agpl). | ||
|
|
||
| from odoo.tests.common import TransactionCase | ||
| from odoo.addons.base.tests.common import BaseCommon | ||
|
|
||
|
|
||
| class TestDiscountDisplay(TransactionCase): | ||
| def test_sale_discount_value(self): | ||
| product1 = self.env["product.product"].create( | ||
| class TestDiscountDisplay(BaseCommon): | ||
| @classmethod | ||
| def setUpClass(cls): | ||
| super().setUpClass() | ||
| cls.product = cls.env["product.product"].create( | ||
| {"name": "Product TEST", "type": "consu"} | ||
| ) | ||
| customer = self.env["res.partner"].create( | ||
| {"name": "Customer TEST", "is_company": False, "email": "test@tes.ttest"} | ||
| ) | ||
| so = self.env["sale.order"].create({"partner_id": customer.id}) | ||
| self.env["sale.order.line"].create( | ||
| {"order_id": so.id, "product_id": product1.id, "price_unit": 30.75} | ||
| cls.so = cls.env["sale.order"].create({"partner_id": cls.partner.id}) | ||
| cls.so_line = cls.env["sale.order.line"].create( | ||
| {"order_id": cls.so.id, "product_id": cls.product.id, "price_unit": 30.75} | ||
| ) | ||
|
|
||
| first_line = so.order_line[0] | ||
| first_line.discount = 10 | ||
| self.assertAlmostEqual(first_line.price_total_no_discount, 35.36) | ||
| self.assertAlmostEqual(first_line.discount_total, 3.53) | ||
| self.assertAlmostEqual(so.discount_total, 3.53) | ||
| self.assertAlmostEqual(so.price_total_no_discount, 35.36) | ||
| def test_sale_discount_value(self): | ||
| self.so_line.discount = 10 | ||
| self.assertAlmostEqual(self.so_line.price_total_no_discount, 35.36) | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Good regression test for the unlink scenario. The comment could be slightly clearer -- consider: "Regression test: compute_discount_total must not write unchanged values, otherwise unlink raises a cache error when flushing triggers recomputation on records being deleted." |
||
| self.assertAlmostEqual(self.so_line.discount_total, 3.53) | ||
| self.assertAlmostEqual(self.so.discount_total, 3.53) | ||
| self.assertAlmostEqual(self.so.price_total_no_discount, 35.36) | ||
|
|
||
| def test_sale_without_discount_value(self): | ||
| product2 = self.env["product.product"].create( | ||
| {"name": "Product TEST", "type": "consu"} | ||
| ) | ||
| customer2 = self.env["res.partner"].create( | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This test exercises the fallback path in Minor: setting |
||
| {"name": "Customer TEST", "is_company": False, "email": "test@tes.ttest"} | ||
| self.assertEqual(self.so_line.price_total_no_discount, self.so_line.price_total) | ||
|
|
||
| def test_unlink_after_compute_discount_total(self): | ||
| # Regression test: compute_discount_total must not write unchanged values, | ||
| # otherwise unlink raises a cache error when flushing triggers | ||
| # recomputation on records being deleted. | ||
| self.so_line.discount = 10 | ||
| self.assertAlmostEqual(self.so_line.price_total_no_discount, 35.36) | ||
| so_id = self.so.id | ||
| self.so.unlink() | ||
| # Check that the sale order is deleted | ||
| self.assertFalse(self.env["sale.order"].browse(so_id).exists()) | ||
|
|
||
| def test_has_discount_with_empty_currency(self): | ||
| # Ensures no error is thrown when currency_id is empty | ||
|
|
||
| # setting self.so_line.currency_id to an empty recordset is a somewhat unusual | ||
| # since currency_id on a sale order line is typically a related field from | ||
| # the order. This is done here to test the behavior of _has_discount | ||
| # when currency_id is not set. | ||
| self.so_line.currency_id = self.env["res.currency"] | ||
| self.assertFalse(self.so_line.currency_id, "Expected currency_id to be empty") | ||
| self.so_line.discount = 0.0 | ||
| self.assertFalse( | ||
| self.so_line._has_discount(), | ||
| "Expected _has_discount to be False when discount is 0 and currency_id is empty", | ||
| ) | ||
| so2 = self.env["sale.order"].create({"partner_id": customer2.id}) | ||
| self.env["sale.order.line"].create( | ||
| {"order_id": so2.id, "product_id": product2.id, "price_unit": 30.75} | ||
|
|
||
| self.so_line.discount = 10 | ||
| self.assertTrue( | ||
| self.so_line._has_discount(), | ||
| "Expected _has_discount to be True when discount is non-zero and currency_id " | ||
| "is empty", | ||
| ) | ||
| first_line = so2.order_line[0] | ||
| self.assertEqual(first_line.price_total_no_discount, first_line.price_total) | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The
float_compareguard is a pragmatic solution to prevent unnecessary cache invalidation (and the unlink regression), but it does add some complexity. A simpler alternative would be to check once at the top of the loop whether any recomputation is needed at all -- though the current approach is more precise since it checks each field independently.Also, note that the
currencyvariable is fetched fromline.order_id.currency_idbut the_has_discountmethod falls back toself.env.company.currency_idwhencurrency_idis falsy. The rounding precision used infloat_comparehere could theoretically differ from the one used in_has_discountiforder_id.currency_idis empty. This is unlikely in practice (a sale order should always have a currency), but worth being aware of for consistency.