-
-
Notifications
You must be signed in to change notification settings - Fork 1.2k
[FIX] sale_procurement_group_by_line: Fix TypeError with kit products in _action_launch_stock_rule #4047
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: 18.0
Are you sure you want to change the base?
[FIX] sale_procurement_group_by_line: Fix TypeError with kit products in _action_launch_stock_rule #4047
Changes from 8 commits
a196172
5ceceab
1942621
2efd45a
221dd36
cc80680
b1ff082
742b3ef
0d6faf0
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 |
|---|---|---|
|
|
@@ -34,12 +34,16 @@ def setUpClass(cls): | |
| "product_uom_qty": 2, | ||
| "price_unit": 50.0, | ||
| }, | ||
| ), | ||
| ( | ||
| 0, | ||
| 0, | ||
| { | ||
| "product_id": cls.product2.id, | ||
| "product_uom_qty": 1, | ||
| "price_unit": 100.0, | ||
| }, | ||
| ) | ||
| ), | ||
| ], | ||
| } | ||
| ) | ||
|
|
@@ -62,13 +66,162 @@ def test_02_amount_total_curr_different_currency(self): | |
| self.sale_order._compute_amount_company() | ||
| amount_total_curr_rounded = round(self.sale_order.amount_total_curr, 2) | ||
| amount_total_converted_rounded = round( | ||
| self.sale_order.amount_total * self.sale_order.currency_rate, 2 | ||
| self.sale_order.amount_total / self.sale_order.currency_rate, 2 | ||
| ) | ||
| self.assertEqual( | ||
| amount_total_curr_rounded, | ||
| amount_total_converted_rounded, | ||
| msg=( | ||
| "Amount in company currency should be converted " | ||
| "using the currency rate." | ||
| "using the currency rate (division, not multiplication)." | ||
| ), | ||
| ) | ||
|
|
||
| def test_03_amount_total_curr_conversion_logic(self): | ||
| """Test specific conversion logic to ensure division is used.""" | ||
| # Set up a specific scenario to test the conversion logic | ||
| self.sale_order.currency_id = self.currency_eur | ||
| # Set a specific currency rate for testing | ||
| self.sale_order.currency_rate = 0.85 # 1 EUR = 0.85 USD | ||
|
|
||
| # Calculate expected result: amount_total / currency_rate | ||
| expected_amount = self.sale_order.amount_total / 0.85 | ||
|
|
||
| self.sale_order._compute_amount_company() | ||
|
|
||
| # Verify the conversion uses division, not multiplication | ||
| self.assertAlmostEqual( | ||
| self.sale_order.amount_total_curr, | ||
| expected_amount, | ||
| places=2, | ||
| msg="Conversion should use division, not multiplication.", | ||
| ) | ||
|
|
||
| # Verify that multiplication would give a different (wrong) result | ||
| wrong_amount = self.sale_order.amount_total * 0.85 | ||
| self.assertNotAlmostEqual( | ||
| self.sale_order.amount_total_curr, | ||
| wrong_amount, | ||
| places=2, | ||
| msg="Multiplication should give different result than correct division.", | ||
| ) | ||
|
||
|
|
||
| def test_04_amount_total_curr_rate_one(self): | ||
| """Test conversion when currency_rate = 1 (should behave like same currency).""" | ||
| self.sale_order.currency_id = self.currency_eur | ||
| self.sale_order.currency_rate = 1.0 | ||
|
|
||
| self.sale_order._compute_amount_company() | ||
|
|
||
| self.assertEqual( | ||
| self.sale_order.amount_total_curr, | ||
| self.sale_order.amount_total, | ||
| "When currency_rate = 1, amount_total_curr should equal amount_total", | ||
| ) | ||
|
|
||
| def test_05_amount_total_curr_extreme_rates(self): | ||
| """Test conversion with very small and very large currency rates.""" | ||
| self.sale_order.currency_id = self.currency_eur | ||
|
|
||
| # Test with very small rate (strong target currency) | ||
| self.sale_order.currency_rate = 0.01 # 1 EUR = 0.01 USD | ||
| self.sale_order._compute_amount_company() | ||
| expected_small = self.sale_order.amount_total / 0.01 | ||
| self.assertAlmostEqual( | ||
| self.sale_order.amount_total_curr, | ||
| expected_small, | ||
| places=2, | ||
| msg="Conversion with very small rate should work correctly", | ||
| ) | ||
|
Comment on lines
+113
to
+126
|
||
|
|
||
| # Test with very large rate (weak target currency) | ||
| self.sale_order.currency_rate = 1000.0 # 1 EUR = 1000 USD | ||
| self.sale_order._compute_amount_company() | ||
| expected_large = self.sale_order.amount_total / 1000.0 | ||
| self.assertAlmostEqual( | ||
| self.sale_order.amount_total_curr, | ||
| expected_large, | ||
| places=2, | ||
| msg="Conversion with very large rate should work correctly", | ||
| ) | ||
|
|
||
| def test_06_amount_total_curr_multiple_orders(self): | ||
| """Test computation with multiple sale orders in one batch.""" | ||
| # Create a second sale order | ||
| sale_order_2 = self.env["sale.order"].create( | ||
| { | ||
| "partner_id": self.partner.id, | ||
| "company_id": self.company.id, | ||
| "currency_id": self.currency_eur.id, | ||
| "order_line": [ | ||
| ( | ||
| 0, | ||
| 0, | ||
| { | ||
| "product_id": self.product1.id, | ||
| "product_uom_qty": 3, | ||
| "price_unit": 75.0, | ||
| }, | ||
| ) | ||
| ], | ||
| } | ||
| ) | ||
|
|
||
| # Set different currency rates for both orders | ||
| # First, change the currency of both orders to force conversion | ||
| # Check what currency the company uses | ||
| if self.company.currency_id == self.currency_usd: | ||
| # Company uses USD, so use EUR for both orders | ||
| self.sale_order.currency_id = self.currency_eur | ||
| sale_order_2.currency_id = self.currency_eur | ||
| else: | ||
| # Company uses something else, use USD for both orders | ||
| self.sale_order.currency_id = self.currency_usd | ||
| sale_order_2.currency_id = self.currency_usd | ||
|
|
||
| self.sale_order.currency_rate = 0.85 | ||
| sale_order_2.currency_rate = 1.20 | ||
|
|
||
| # Compute for both orders in batch | ||
| orders = self.sale_order + sale_order_2 | ||
| orders._compute_amount_company() | ||
|
|
||
| # Verify both orders computed correctly | ||
| # Use the actual amount_total values from the orders | ||
| expected_1 = self.sale_order.amount_total / 0.85 | ||
| expected_2 = sale_order_2.amount_total / 1.20 | ||
|
|
||
| self.assertAlmostEqual( | ||
| self.sale_order.amount_total_curr, | ||
| expected_1, | ||
| places=2, | ||
| msg="First order should compute correctly in batch", | ||
| ) | ||
| self.assertAlmostEqual( | ||
| sale_order_2.amount_total_curr, | ||
| expected_2, | ||
| places=2, | ||
| msg="Second order should compute correctly in batch", | ||
| ) | ||
|
|
||
| def test_07_amount_total_curr_edge_cases(self): | ||
| """Test edge cases like zero amount_total and extreme values.""" | ||
| self.sale_order.currency_id = self.currency_eur | ||
|
|
||
| # Test with zero amount_total - set all line prices to zero | ||
| for line in self.sale_order.order_line: | ||
| line.price_unit = 0.0 | ||
| self.sale_order.currency_rate = 0.85 | ||
|
|
||
| self.sale_order._compute_amount_company() | ||
|
|
||
| self.assertEqual( | ||
| self.sale_order.amount_total_curr, | ||
| 0.0, | ||
| "Zero amount_total should result in zero amount_total_curr", | ||
| ) | ||
|
|
||
| # Reset to normal values | ||
| self.sale_order.order_line[0].price_unit = 50.0 | ||
| if len(self.sale_order.order_line) > 1: | ||
| self.sale_order.order_line[1].price_unit = 100.0 | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -46,9 +46,24 @@ def _action_launch_stock_rule(self, previous_product_uom_qty=False): | |
| or line.product_id.type != "consu" | ||
| ): | ||
| continue | ||
| qty = line._get_qty_procurement(previous_product_uom_qty) or 0.0 | ||
|
|
||
| # Get quantity with safety for None values | ||
| qty = line._get_qty_procurement(previous_product_uom_qty) | ||
|
|
||
| # Handle kit products: skip procurement for kits | ||
| if line.product_id.type == "kit": | ||
| continue | ||
|
||
|
|
||
| # Ensure qty is not None | ||
| qty_to_compare = qty or 0.0 | ||
| product_uom_qty_to_compare = line.product_uom_qty or 0.0 | ||
|
|
||
| if ( | ||
| float_compare(qty, line.product_uom_qty, precision_digits=precision) | ||
| float_compare( | ||
| qty_to_compare, | ||
| product_uom_qty_to_compare, | ||
| precision_digits=precision, | ||
| ) | ||
| == 0 | ||
| ): | ||
| continue | ||
|
|
||
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -142,3 +142,43 @@ def test_06_update_sale_order_line_respect_procurement_group(self): | |||||||||||||||||||||||||||||||||
| self.sale.order_line[1].product_uom_qty += 1 | ||||||||||||||||||||||||||||||||||
| self.assertEqual(self.sale.order_line[1].procurement_group_id, proc_group) | ||||||||||||||||||||||||||||||||||
| self.assertEqual(len(self.line1.move_ids), 1) | ||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||
| def test_07_kit_product_no_procurement_error(self): | ||||||||||||||||||||||||||||||||||
| """Test that kit products don't cause TypeError in _action_launch_stock_rule. | ||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||
| This test reproduces the bug described in issue #3890 where products | ||||||||||||||||||||||||||||||||||
| of type 'kit' could cause TypeError with None values | ||||||||||||||||||||||||||||||||||
| """ | ||||||||||||||||||||||||||||||||||
| # Create a kit product | ||||||||||||||||||||||||||||||||||
| kit_product = self.product_model.create( | ||||||||||||||||||||||||||||||||||
| { | ||||||||||||||||||||||||||||||||||
| "name": "Test Kit Product", | ||||||||||||||||||||||||||||||||||
| "categ_id": self.product_ctg.id, | ||||||||||||||||||||||||||||||||||
| "type": "kit", | ||||||||||||||||||||||||||||||||||
| "is_storable": True, | ||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||
| "is_storable": True, |
Outdated
Copilot
AI
Nov 29, 2025
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.
This assertion will always fail because the kit product check on line 54 of sale.py is unreachable (it's filtered out by the type != "consu" check on line 46). The kit product will never reach the procurement logic, so it won't have a procurement_group_id, but not for the reason the test expects (being "skipped in our fix").
Outdated
Copilot
AI
Nov 29, 2025
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.
Using a try-except block to test for bugs is not a best practice for unit tests. If the test is meant to validate that the bug is fixed, it should simply call action_confirm() and assert the expected behavior. The try-except pattern makes the test pass even if the bug still exists (by explicitly failing with a message), which is confusing. Consider refactoring to:
# This should not raise TypeError
self.sale.action_confirm()
self.assertEqual(self.sale.state, "sale")
# Kit product should not have procurement group
self.assertFalse(kit_line.procurement_group_id)| try: | |
| self.sale.action_confirm() | |
| # If we get here, the bug is fixed | |
| self.assertEqual(self.sale.state, "sale") | |
| # Kit product should not have procurement group (skipped in our fix) | |
| self.assertFalse(kit_line.procurement_group_id) | |
| except TypeError as e: | |
| if "unsupported operand type(s) for" in str(e): | |
| self.fail(f"Bug #3890 reproduced: {e}") | |
| else: | |
| # Re-raise if it's a different TypeError | |
| raise | |
| self.sale.action_confirm() | |
| self.assertEqual(self.sale.state, "sale") | |
| # Kit product should not have procurement group (skipped in our fix) | |
| self.assertFalse(kit_line.procurement_group_id) |
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 change from multiplication to division fixes the currency conversion logic, but there's no protection against division by zero. If
order.currency_rateis 0 or very close to 0, this will raise a ZeroDivisionError. Consider adding a check: