[FIX] product: fix dynamic product variant archiving

Issue

	- Install Sales with Product Configurator
	- Create a product with 2 dynamic variants:
		- Color: blue, red
		- Size: S, M
	- Create a quotation with that product (any variant)
	- Sales > Products > Products Variants

	Your product variant is there

	- Modify your product template and add "green"
	  to colors
	- Sales > Products > Products Variants

	Your product variant is archived

Cause

	The logic which determines which variant
	should not be archived is only applied
	on products that don't have dynamic variants

	See https://github.com/odoo/odoo/blob/820fe9a025e34e81368a189ee295b4abcf9fcbda/addons/product/models/product_template.py#L576

Solution

	Do a separate logic for dynamic variants that doesn't compute
	all possible combinations.

	But being sure that it:
		- doesn't archive when adding/removing a value
		  to an existing attribute line
		- archive when adding/removing a new attribute line

OPW-2293346

closes odoo/odoo#55016

X-original-commit: 8517fdfa2605e8d15f5e38f2074ece7ba543d56b
Signed-off-by: Sébastien Theys (seb) <seb@odoo.com>

3 files changed, 174 insertions, 28 deletions

diff --git a/addons/product/models/product.py b/addons/product/models/product.py
index c11fa3aac44..2349ac40194 100644
--- a/addons/product/models/product.py
+++ b/addons/product/models/product.py
@@ -678,9 +678,6 @@ class ProductProduct(models.Model):
 
         See `_is_combination_possible` for more information.
 
-        This will always exclude variants for templates that have `no_variant`
-        attributes because the variant itself will not be the full combination.
-
         :param parent_combination: combination from which `self` is an
             optional or accessory product.
         :type parent_combination: recordset `product.template.attribute.value`
@@ -689,7 +686,7 @@ class ProductProduct(models.Model):
         :rtype: bool
         """
         self.ensure_one()
-        return self.product_tmpl_id._is_combination_possible(self.product_template_attribute_value_ids, parent_combination=parent_combination)
+        return self.product_tmpl_id._is_combination_possible(self.product_template_attribute_value_ids, parent_combination=parent_combination, ignore_no_variant=True)
 
     def toggle_active(self):
         """ Archiving related product.template if there is only one active product.product """
diff --git a/addons/product/models/product_template.py b/addons/product/models/product_template.py
index bf28f6e5acb..2039468696a 100644
--- a/addons/product/models/product_template.py
+++ b/addons/product/models/product_template.py
@@ -572,7 +572,7 @@ class ProductTemplate(models.Model):
         for tmpl_id in self:
             lines_without_no_variants = tmpl_id.valid_product_template_attribute_line_ids._without_no_variant_attributes()
 
-            all_variants = tmpl_id.with_context(active_test=False).product_variant_ids.sorted('active')
+            all_variants = tmpl_id.with_context(active_test=False).product_variant_ids.sorted(lambda p: (p.active, -p.id))
 
             current_variants_to_create = []
             current_variants_to_activate = Product
@@ -591,6 +591,11 @@ class ProductTemplate(models.Model):
                     ):
                         variant.product_template_attribute_value_ids = combination
 
+            # Set containing existing `product.template.attribute.value` combination
+            existing_variants = {
+                variant.product_template_attribute_value_ids: variant for variant in all_variants
+            }
+
             # Determine which product variants need to be created based on the attribute
             # configuration. If any attribute is set to generate variants dynamically, skip the
             # process.
@@ -602,10 +607,6 @@ class ProductTemplate(models.Model):
                 all_combinations = itertools.product(*[
                     ptal.product_template_value_ids._only_active() for ptal in lines_without_no_variants
                 ])
-                # Set containing existing `product.template.attribute.value` combination
-                existing_variants = {
-                    variant.product_template_attribute_value_ids: variant for variant in all_variants
-                }
                 # For each possible variant, create if it doesn't exist yet.
                 for combination_tuple in all_combinations:
                     combination = self.env['product.template.attribute.value'].concat(*combination_tuple)
@@ -625,6 +626,16 @@ class ProductTemplate(models.Model):
                 variants_to_create += current_variants_to_create
                 variants_to_activate += current_variants_to_activate
 
+            else:
+                for variant in existing_variants.values():
+                    is_combination_possible = self._is_combination_possible_by_config(
+                        combination=variant.product_template_attribute_value_ids,
+                        ignore_no_variant=True,
+                    )
+                    if is_combination_possible:
+                        current_variants_to_activate += variant
+                variants_to_activate += current_variants_to_activate
+
             variants_to_unlink += all_variants - current_variants_to_activate
 
         if variants_to_activate:
@@ -669,8 +680,8 @@ class ProductTemplate(models.Model):
         """Return the existing variants that are possible.
 
         For dynamic attributes, it will only return the variants that have been
-        created already. For no_variant attributes, it will return an empty
-        recordset because the variants themselves are not a full combination.
+        created already.
+
         If there are a lot of variants, this method might be slow. Even if there
         aren't too many variants, for performance reasons, do not call this
         method in a loop over the product templates.
@@ -797,7 +808,41 @@ class ProductTemplate(models.Model):
             for attribute_value in all_product_attribute_values
         }
 
-    def _is_combination_possible(self, combination, parent_combination=None):
+    def _is_combination_possible_by_config(self, combination, ignore_no_variant=False):
+        """Return whether the given combination is possible according to the config of attributes on the template
+
+        :param combination: the combination to check for possibility
+        :type combination: recordset `product.template.attribute.value`
+
+        :param ignore_no_variant: whether no_variant attributes should be ignored
+        :type ignore_no_variant: bool
+
+        :return: wether the given combination is possible according to the config of attributes on the template
+        :rtype: bool
+        """
+        self.ensure_one()
+
+        attribute_lines = self.valid_product_template_attribute_line_ids
+
+        if ignore_no_variant:
+            attribute_lines = attribute_lines._without_no_variant_attributes()
+
+        if len(combination) != len(attribute_lines):
+            # number of attribute values passed is different than the
+            # configuration of attributes on the template
+            return False
+
+        if attribute_lines != combination.attribute_line_id:
+            # combination has different attributes than the ones configured on the template
+            return False
+
+        if not (attribute_lines.product_template_value_ids._only_active() >= combination):
+            # combination has different values than the ones configured on the template
+            return False
+
+        return True
+
+    def _is_combination_possible(self, combination, parent_combination=None, ignore_no_variant=False):
         """
         The combination is possible if it is not excluded by any rule
         coming from the current template, not excluded by any rule from the
@@ -813,6 +858,9 @@ class ProductTemplate(models.Model):
         :param combination: the combination to check for possibility
         :type combination: recordset `product.template.attribute.value`
 
+        :param ignore_no_variant: whether no_variant attributes should be ignored
+        :type ignore_no_variant: bool
+
         :param parent_combination: combination from which `self` is an
             optional or accessory product.
         :type parent_combination: recordset `product.template.attribute.value`
@@ -822,17 +870,7 @@ class ProductTemplate(models.Model):
         """
         self.ensure_one()
 
-        if len(combination) != len(self.valid_product_template_attribute_line_ids):
-            # number of attribute values passed is different than the
-            # configuration of attributes on the template
-            return False
-
-        if self.valid_product_template_attribute_line_ids != combination.attribute_line_id:
-            # combination has different attributes than the ones configured on the template
-            return False
-
-        if self.valid_product_template_attribute_line_ids.product_template_value_ids._only_active() < combination:
-            # combination has different values than the ones configured on the template
+        if not self._is_combination_possible_by_config(combination, ignore_no_variant):
             return False
 
         variant = self._get_variant_for_combination(combination)
diff --git a/addons/product/tests/test_variants.py b/addons/product/tests/test_variants.py
index b5a09d9f736..a0ee4134f0a 100644
--- a/addons/product/tests/test_variants.py
+++ b/addons/product/tests/test_variants.py
@@ -907,11 +907,7 @@ class TestVariantsArchive(common.TestProductCommon):
             'value_ids': [(0, False, {'name': 'ValueDynamic'})],
         })
         template = self.env['product.template'].create({
-            'name': 'cimanyd'
-        })
-        self.assertEqual(len(template.product_variant_ids), 1)
-
-        template.write({
+            'name': 'cimanyd',
             'attribute_line_ids': [(0, False, {
                 'attribute_id': dynamic_attr.id,
                 'value_ids': [(4, dynamic_attr.value_ids[0].id, False)],
@@ -943,6 +939,121 @@ class TestVariantsArchive(common.TestProductCommon):
         self.assertEqual(variant.uom_po_id, units)
         self.assertEqual(template.uom_po_id, units)
 
+    def test_dynamic_attributes_archiving(self):
+        Product = self.env['product.product']
+        ProductAttribute = self.env['product.attribute']
+        ProductAttributeValue = self.env['product.attribute.value']
+
+        # Patch unlink method to force archiving instead deleting
+        def unlink(self):
+            self.active = False
+        Product._patch_method('unlink', unlink)
+
+        # Creating attributes
+        pa_color = ProductAttribute.create({'sequence': 1, 'name': 'color', 'create_variant': 'dynamic'})
+        color_values = ProductAttributeValue.create([{
+            'name': n,
+            'sequence': i,
+            'attribute_id': pa_color.id,
+        } for i, n in enumerate(['white', 'black'])])
+        pav_color_white = color_values[0]
+        pav_color_black = color_values[1]
+
+        pa_size = ProductAttribute.create({'sequence': 2, 'name': 'size', 'create_variant': 'dynamic'})
+        size_values = ProductAttributeValue.create([{
+            'name': n,
+            'sequence': i,
+            'attribute_id': pa_size.id,
+        } for i, n in enumerate(['s', 'm'])])
+        pav_size_s = size_values[0]
+        pav_size_m = size_values[1]
+
+        pa_material = ProductAttribute.create({'sequence': 3, 'name': 'material', 'create_variant': 'no_variant'})
+        material_values = ProductAttributeValue.create([{
+            'name': 'Wood',
+            'sequence': 1,
+            'attribute_id': pa_material.id,
+        }])
+        pav_material_wood = material_values[0]
+
+        # Define a template with only color attribute & white value
+        template = self.env['product.template'].create({
+            'name': 'test product',
+            'attribute_line_ids': [(0, 0, {
+                'attribute_id': pa_color.id,
+                'value_ids': [(6, 0, [pav_color_white.id])],
+            })],
+        })
+
+        # Create a variant (because of dynamic attribute)
+        ptav_white = self.env['product.template.attribute.value'].search([
+            ('attribute_line_id', '=', template.attribute_line_ids.id),
+            ('product_attribute_value_id', '=', pav_color_white.id)
+        ])
+        product_white = template._create_product_variant(ptav_white)
+
+        # Adding a new value to an existing attribute should not archive the variant
+        template.write({
+            'attribute_line_ids': [(1, template.attribute_line_ids[0].id, {
+                'attribute_id': pa_color.id,
+                'value_ids': [(4, pav_color_black.id, False)],
+            })]
+        })
+        self.assertTrue(product_white.active)
+
+        # Removing an attribute value should archive the product using it
+        template.write({
+            'attribute_line_ids': [(1, template.attribute_line_ids[0].id, {
+                'value_ids': [(3, pav_color_white.id, 0)],
+            })]
+        })
+        self.assertFalse(product_white.active)
+        self.assertFalse(template._is_combination_possible_by_config(
+            combination=product_white.product_template_attribute_value_ids,
+            ignore_no_variant=True,
+        ))
+
+        # Creating a product with the same attributes for testing duplicates
+        product_white_duplicate = Product.create({
+            'product_tmpl_id': template.id,
+            'product_template_attribute_value_ids': [(6, 0, [ptav_white.id])],
+            'active': False,
+        })
+        # Reset archiving for the next assert
+        template.write({
+            'attribute_line_ids': [(1, template.attribute_line_ids[0].id, {
+                'value_ids': [(4, pav_color_white.id, 0)],
+            })]
+        })
+        self.assertTrue(product_white.active)
+        self.assertFalse(product_white_duplicate.active)
+
+        # Adding a new attribute should archive the old variant
+        template.write({
+            'attribute_line_ids': [(0, 0, {
+                'attribute_id': pa_size.id,
+                'value_ids': [(6, 0, [pav_size_s.id, pav_size_m.id])],
+            })]
+        })
+        self.assertFalse(product_white.active)
+
+        # Reset archiving for the next assert
+        template.write({
+            'attribute_line_ids': [(3, template.attribute_line_ids[1].id, 0)]
+        })
+        self.assertTrue(product_white.active)
+
+        # Adding a no_variant attribute should not archive the product
+        template.write({
+            'attribute_line_ids': [(0, 0, {
+                'attribute_id': pa_material.id,
+                'value_ids': [(6, 0, [pav_material_wood.id])],
+            })]
+        })
+        self.assertTrue(product_white.active)
+
+        Product._revert_method('unlink')
+
     def _update_color_vars(self, ptal):
         self.ptal_color = ptal
         self.assertEqual(self.ptal_color.attribute_id, self.pa_color)