From c84cc40063a83a0ada6651235cc7495dba144b43 Mon Sep 17 00:00:00 2001
From: Richard deMeester <richard@willowit.com.au>
Date: Mon, 29 May 2017 17:33:02 +1000
Subject: [PATCH 1/6] [FIX] Wizard Attributes

Wizard attributes (readonly, required, invisible) now controlled
by 3 virtual fields each, allowing far greater control.  This fixes
a number of issues:

* Attributes with conditional domains might be required, or might
    not be
* Attributes with conditional domains marked as required can now
    be validated if the domain is empty.
* Readonly values can be better detected when the values are not
    passed from the front end.
* Super complex domains can now be used to drive the readonly
    and required values, ands and ors and inherited, as the
    driver is now purely the list of valid ids.
* Easier modification of behaviour in custom circumstances
    with inheritance possible to drive these virtual columns.
---
 product_configurator/models/product.py        |  12 +-
 .../wizard/product_configurator.py            | 344 ++++++++++++------
 .../wizard/product_configurator_view.xml      |   1 +
 3 files changed, 239 insertions(+), 118 deletions(-)

diff --git a/product_configurator/models/product.py b/product_configurator/models/product.py
index 9a6bfe56..f89a9982 100644
--- a/product_configurator/models/product.py
+++ b/product_configurator/models/product.py
@@ -457,6 +457,9 @@ def validate_configuration(self, value_ids, custom_vals=None, final=True):
         if custom_vals is None:
             custom_vals = {}
 
+        # Check if all the values passed are not restricted
+        avail_val_ids = self.values_available(value_ids, value_ids)
+
         for line in self.attribute_line_ids:
             # Validate custom values
             attr = line.attribute_id
@@ -466,11 +469,12 @@ def validate_configuration(self, value_ids, custom_vals=None, final=True):
                 common_vals = set(value_ids) & set(line.value_ids.ids)
                 custom_val = custom_vals.get(attr.id)
                 if line.required and not common_vals and not custom_val:
-                    # TODO: Verify custom value type to be correct
-                    return False
+                    # If there are possible values, then enforce
+                    # the "required" status.
+                    if set(line.value_ids.ids) & set(avail_val_ids):
+                        # TODO: Verify custom value type to be correct
+                        return False
 
-        # Check if all all the values passed are not restricted
-        avail_val_ids = self.values_available(value_ids, value_ids)
         if set(value_ids) - set(avail_val_ids):
             return False
 
diff --git a/product_configurator_wizard/wizard/product_configurator.py b/product_configurator_wizard/wizard/product_configurator.py
index 5e0055d0..afe2e08a 100644
--- a/product_configurator_wizard/wizard/product_configurator.py
+++ b/product_configurator_wizard/wizard/product_configurator.py
@@ -1,6 +1,7 @@
 # -*- coding: utf-8 -*-
 
 from lxml import etree
+import ast
 
 from odoo.osv import orm
 from odoo import models, fields, api, _
@@ -21,6 +22,9 @@ class ProductConfigurator(models.TransientModel):
     # Prefix for the dynamicly injected fields
     field_prefix = '__attribute-'
     custom_field_prefix = '__custom-'
+    ro_field_prefix = '__ro-'
+    reqd_field_prefix = '__reqd-'
+    invis_field_prefix = '__invis-'
 
     # TODO: Since the configuration process can take a bit of time
     # depending on complexity and AFK time we must increase the lifespan
@@ -78,13 +82,13 @@ def onchange_product_tmpl(self):
 
     def get_onchange_domains(self, values, cfg_val_ids):
         """Generate domains to be returned by onchange method in order
-        to restrict the availble values of dynamically inserted fields
+        to restrict the available values of dynamically inserted fields
 
-        :param values: values argument passed to onchance wrapper
+        :param values: values argument passed to onchange wrapper
         :cfg_val_ids: current configuration passed as a list of value_ids
         (usually in the form of db value_ids + interface value_ids)
 
-        :returns: a dictionary of domains returned by onchance method
+        :returns: a dictionary of domains returned by onchange method
         """
         domains = {}
         for line in self.product_tmpl_id.attribute_line_ids.sorted():
@@ -109,7 +113,20 @@ def get_onchange_domains(self, values, cfg_val_ids):
                     continue
         return domains
 
-    def get_form_vals(self, dynamic_fields, domains):
+    def get_support_fields(self, k, available_val_ids, tmpl_attribute):
+        vals = {}
+        attribute_id = int(k.replace(self.field_prefix, ''))
+        vals[self.ro_field_prefix + str(attribute_id)] = \
+            len(available_val_ids) == 0
+        if tmpl_attribute.required:
+            required = len(available_val_ids) > 0
+        else:
+            required = False
+        vals[self.reqd_field_prefix + str(attribute_id)] = required
+        vals[self.invis_field_prefix + str(attribute_id)] = False
+        return vals
+
+    def get_form_vals(self, dynamic_fields, domains, cfg_step):
         """Generate a dictionary to return new values via onchange method.
         Domains hold the values available, this method enforces these values
         if a selection exists in the view that is not available anymore.
@@ -121,19 +138,31 @@ def get_form_vals(self, dynamic_fields, domains):
         """
         vals = {}
 
-        dynamic_fields = {k: v for k, v in dynamic_fields.iteritems() if v}
+        dynamic_fields = dynamic_fields.copy()
 
         for k, v in dynamic_fields.iteritems():
-            if not v:
-                continue
+            attribute_id = int(k.replace(self.field_prefix, ''))
+            tmpl_attribute = self.product_tmpl_id.attribute_line_ids.filtered(
+                lambda a: a.attribute_id.id == attribute_id)
             available_val_ids = domains[k][0][2]
-            if isinstance(v, list):
+
+            vals.update(
+                self.get_support_fields(k, available_val_ids, tmpl_attribute)
+            )
+
+            if v and isinstance(v, list):
                 value_ids = list(set(v[0][2]) & set(available_val_ids))
                 dynamic_fields.update({k: value_ids})
                 vals[k] = [[6, 0, value_ids]]
-            elif v not in available_val_ids:
+            elif v and v not in available_val_ids:
                 dynamic_fields.update({k: None})
                 vals[k] = None
+                v = None
+            if not v:
+                if tmpl_attribute.required and not tmpl_attribute.multi and \
+                        len(available_val_ids) == 1:
+                    dynamic_fields.update({k: available_val_ids[0]})
+                    vals[k] = available_val_ids[0]
 
         product_img = self.product_tmpl_id.get_config_image_obj(
             dynamic_fields.values())
@@ -159,12 +188,7 @@ def onchange(self, values, field_name, field_onchange):
         view_val_ids = set()
         view_attribute_ids = set()
 
-        try:
-            cfg_step_id = int(self.state)
-            cfg_step = self.product_tmpl_id.config_step_line_ids.filtered(
-                lambda x: x.id == cfg_step_id)
-        except:
-            cfg_step = self.env['product.config.step.line']
+        cfg_step = self.get_cfg_step()
 
         dynamic_fields = {
             k: v for k, v in values.iteritems() if k.startswith(
@@ -196,7 +220,38 @@ def onchange(self, values, field_name, field_onchange):
         cfg_val_ids = cfg_vals.ids + list(view_val_ids)
 
         domains = self.get_onchange_domains(values, cfg_val_ids)
-        vals = self.get_form_vals(dynamic_fields, domains)
+        vals = self.get_form_vals(dynamic_fields, domains, cfg_step)
+
+        # See if any of the dynamic_fields were modified by the get_form_vals
+        # (cleared or set), they may change domains!
+        modified_dynamics = {k: v
+                             for k, v in vals.iteritems()
+                             if k in dynamic_fields}
+        while modified_dynamics:
+            dynamic_fields.update(modified_dynamics)
+            for k, v in modified_dynamics.iteritems():
+                attr_id = int(k.split(self.field_prefix)[1])
+                # First take out what the value may have been before
+                view_val_ids -= set(self.env['product.attribute.value'].search(
+                    [('attribute_id', '=', attr_id)]).ids)
+                # And if set, now put it back in
+                if v:
+                    if isinstance(v, list):
+                        view_val_ids |= set(v[0][2])
+                    elif isinstance(v, int):
+                        view_val_ids.add(v)
+
+            cfg_val_ids = cfg_vals.ids + list(view_val_ids)
+
+            domains = self.get_onchange_domains(values, cfg_val_ids)
+            nvals = self.get_form_vals(dynamic_fields, domains, cfg_step)
+            # Stop possible recursion by not including values which have
+            # previously looped
+            modified_dynamics = {k: v
+                                 for k, v in nvals.iteritems()
+                                 if k in dynamic_fields and k not in vals}
+            vals.update(nvals)
+
         return {'value': vals, 'domain': domains}
 
     attribute_line_ids = fields.One2many(
@@ -219,12 +274,13 @@ def onchange(self, values, field_name, field_onchange):
         comodel_name='product.product',
         readonly=True,
         string='Product Variant',
-        help='Set only when re-configuring a existing variant'
+        help='Set only when re-configuring an existing variant'
     )
     product_img = fields.Binary(
         compute='_compute_cfg_image',
         readonly=True
     )
+    stored_support_vals = fields.Char()
     state = FreeSelection(
         selection='get_state_selection',
         default='select',
@@ -259,19 +315,20 @@ def fields_get(self, allfields=None, attributes=None):
         if not wiz.product_tmpl_id:
             return res
 
-        cfg_step_lines = wiz.product_tmpl_id.config_step_line_ids
-
-        try:
-            # Get only the attribute lines for the next step if defined
-            active_step_line = cfg_step_lines.filtered(
-                lambda l: l.id == int(active_step_id))
-            if active_step_line:
-                attribute_lines = active_step_line.attribute_line_ids
-            else:
-                attribute_lines = wiz.product_tmpl_id.attribute_line_ids
-        except:
-            # If no configuration steps exist then get all attribute lines
-            attribute_lines = wiz.product_tmpl_id.attribute_line_ids
+        # At the moment, this is not being used, so let's not call it
+        # unnecessarily
+#         cfg_step_lines = wiz.product_tmpl_id.config_step_line_ids
+#         try:
+#             # Get only the attribute lines for the next step if defined
+#             active_step_line = cfg_step_lines.filtered(
+#                 lambda l: l.id == int(active_step_id))
+#             if active_step_line:
+#                 attribute_lines = active_step_line.attribute_line_ids
+#             else:
+#                 attribute_lines = wiz.product_tmpl_id.attribute_line_ids
+#         except:
+#             # If no configuration steps exist then get all attribute lines
+#             attribute_lines = wiz.product_tmpl_id.attribute_line_ids
 
         attribute_lines = wiz.product_tmpl_id.attribute_line_ids
 
@@ -310,7 +367,7 @@ def fields_get(self, allfields=None, attributes=None):
                 field_types = self.env['ir.model.fields']._get_field_types()
 
                 if attribute.custom_type:
-                    custom_type = line.attribute_id.custom_type
+                    custom_type = attribute.custom_type
                     # TODO: Rename int to integer in values
                     if custom_type == 'int':
                         field_type = 'integer'
@@ -320,7 +377,7 @@ def fields_get(self, allfields=None, attributes=None):
                 # TODO: Implement custom string on custom attribute
                 res[self.custom_field_prefix + str(attribute.id)] = dict(
                     default_attrs,
-                    string="Custom",
+                    string='%s Custom' % (attribute.name,),
                     type=field_type,
                     sequence=line.sequence,
                 )
@@ -331,11 +388,24 @@ def fields_get(self, allfields=None, attributes=None):
                 default_attrs,
                 type='many2many' if line.multi else 'many2one',
                 domain=[('id', 'in', value_ids)],
-                string=line.attribute_id.name,
+                string=attribute.name,
                 relation='product.attribute.value',
                 sequence=line.sequence,
             )
 
+            res[self.ro_field_prefix + str(attribute.id)] = dict(
+                default_attrs,
+                type='boolean',
+            )
+            res[self.reqd_field_prefix + str(attribute.id)] = dict(
+                default_attrs,
+                type='boolean',
+            )
+            res[self.invis_field_prefix + str(attribute.id)] = dict(
+                default_attrs,
+                type='boolean',
+            )
+
         return res
 
     @api.model
@@ -361,17 +431,40 @@ def fields_view_get(self, view_id=None, view_type='form',
             k: v for k, v in fields.iteritems() if k.startswith(
                 self.field_prefix) or k.startswith(self.custom_field_prefix)
         }
+        support_fields = {
+            k: v for k, v in fields.iteritems() if
+            k.startswith(self.ro_field_prefix) or
+            k.startswith(self.reqd_field_prefix) or
+            k.startswith(self.invis_field_prefix)
+        }
 
         res['fields'].update(dynamic_fields)
-        mod_view = self.add_dynamic_fields(res, dynamic_fields, wiz)
+        res['fields'].update(support_fields)
+        mod_view = self.add_dynamic_fields(
+            res, dynamic_fields, support_fields, wiz)
 
         # Update result dict from super with modified view
         res.update({'arch': etree.tostring(mod_view)})
 
+        wiz_vals = wiz.with_context(from_fvg=True).read(
+            dynamic_fields.keys())[0]
+        dynamic_field_vals = {
+            k: wiz_vals.get(
+                k, [] if v['type'] == 'many2many' else False
+                )
+            for k, v in fields.iteritems()
+            if k.startswith(self.field_prefix)
+        }
+        domains = {k: dynamic_fields[k]['domain']
+                   for k in dynamic_field_vals.keys()}
+        cfg_step = wiz.get_cfg_step()
+        vals = wiz.get_form_vals(dynamic_field_vals, domains, cfg_step)
+        if vals:
+            wiz.write(vals)
         return res
 
     @api.model
-    def add_dynamic_fields(self, res, dynamic_fields, wiz):
+    def add_dynamic_fields(self, res, dynamic_fields, support_fields, wiz):
         """ Create the configuration view using the dynamically generated
             fields in fields_get()
         """
@@ -405,6 +498,9 @@ def add_dynamic_fields(self, res, dynamic_fields, wiz):
             attribute_id = attr_line.attribute_id.id
             field_name = self.field_prefix + str(attribute_id)
             custom_field = self.custom_field_prefix + str(attribute_id)
+            ro_field = self.ro_field_prefix + str(attribute_id)
+            reqd_field = self.reqd_field_prefix + str(attribute_id)
+            invis_field = self.invis_field_prefix + str(attribute_id)
 
             # Check if the attribute line has been added to the db fields
             if field_name not in dynamic_fields:
@@ -415,61 +511,27 @@ def add_dynamic_fields(self, res, dynamic_fields, wiz):
 
             # attrs property for dynamic fields
             attrs = {
-                'readonly': ['|'],
+                'readonly': [],
                 'required': [],
-                'invisible': ['|']
+                'invisible': []
             }
 
-            if config_steps:
-                cfg_step_ids = [str(id) for id in config_steps.ids]
-                attrs['invisible'].append(('state', 'not in', cfg_step_ids))
-                attrs['readonly'].append(('state', 'not in', cfg_step_ids))
-
-                # If attribute is required make it so only in the proper step
-                if attr_line.required:
-                    attrs['required'].append(('state', 'in', cfg_step_ids))
-
             if attr_line.custom:
-                pass
                 # TODO: Implement restrictions for ranges
+                pass
 
-            config_lines = wiz.product_tmpl_id.config_line_ids
-            dependencies = config_lines.filtered(
-                lambda cl: cl.attribute_line_id == attr_line)
-
-            # If an attribute field depends on another field from the same
-            # configuration step then we must use attrs to enable/disable the
-            # required and readonly depending on the value entered in the
-            # dependee
-
-            if attr_line.value_ids <= dependencies.mapped('value_ids'):
-                attr_depends = {}
-                domain_lines = dependencies.mapped('domain_id.domain_line_ids')
-                for domain_line in domain_lines:
-                    attr_id = domain_line.attribute_id.id
-                    attr_field = self.field_prefix + str(attr_id)
-                    attr_lines = wiz.product_tmpl_id.attribute_line_ids
-                    # If the fields it depends on are not in the config step
-                    if config_steps and str(attr_line.id) != wiz.state:
-                        continue
-                    if attr_field not in attr_depends:
-                        attr_depends[attr_field] = set()
-                    if domain_line.condition == 'in':
-                        attr_depends[attr_field] |= set(
-                            domain_line.value_ids.ids)
-                    elif domain_line.condition == 'not in':
-                        val_ids = attr_lines.filtered(
-                            lambda l: l.attribute_id.id == attr_id).value_ids
-                        val_ids = val_ids - domain_line.value_ids
-                        attr_depends[attr_field] |= set(val_ids.ids)
-
-                for dependee_field, val_ids in attr_depends.iteritems():
-                    if not val_ids:
-                        continue
-                    attrs['readonly'].append(
-                        (dependee_field, 'not in', list(val_ids)))
-                    attrs['required'].append(
-                        (dependee_field, 'in', list(val_ids)))
+            attrs['readonly'].append(
+                (ro_field, '=', True))
+            attrs['required'].append(
+                (reqd_field, '=', True))
+            attrs['invisible'].append(
+                (invis_field, '=', True))
+            if config_steps:
+                cfg_step_ids = [str(id) for id in config_steps.ids]
+                attrs['invisible'] = ['|'] + attrs['invisible'][:] +\
+                    [('state', 'not in', cfg_step_ids)]
+                attrs['required'] = ['&'] + attrs['required'][:] +\
+                    [('state', 'in', cfg_step_ids)]
 
             # Create the new field in the view
             node = etree.Element(
@@ -486,9 +548,6 @@ def add_dynamic_fields(self, res, dynamic_fields, wiz):
                 })
             )
 
-            if attr_line.required and not config_steps:
-                node.attrib['required'] = '1'
-
             field_type = dynamic_fields[field_name].get('type')
             if field_type == 'many2many':
                 node.attrib['widget'] = 'many2many_tags'
@@ -508,13 +567,10 @@ def add_dynamic_fields(self, res, dynamic_fields, wiz):
                 else:
                     field_val = custom_option_id
 
-                attrs['readonly'] += [(field_name, '!=', field_val)]
-                attrs['invisible'] += [(field_name, '!=', field_val)]
+                attrs['invisible'] = ['|'] + attrs['invisible'][:] + \
+                    [(field_name, '!=', field_val)]
                 attrs['required'] += [(field_name, '=', field_val)]
 
-                if config_steps:
-                    attrs['required'] += [('state', 'in', cfg_step_ids)]
-
                 # TODO: Add a field2widget mapper
                 if attr_line.attribute_id.custom_type == 'color':
                     widget = 'color'
@@ -527,12 +583,48 @@ def add_dynamic_fields(self, res, dynamic_fields, wiz):
                 orm.setup_modifiers(node)
                 xml_dynamic_form.append(node)
 
+            node = etree.Element(
+                "field",
+                name=ro_field,
+                invisible='1',
+            )
+            orm.setup_modifiers(node)
+            xml_dynamic_form.append(node)
+
+            node = etree.Element(
+                "field",
+                name=reqd_field,
+                invisible='1',
+            )
+            orm.setup_modifiers(node)
+            xml_dynamic_form.append(node)
+
+            node = etree.Element(
+                "field",
+                name=invis_field,
+                invisible='1',
+            )
+            orm.setup_modifiers(node)
+            xml_dynamic_form.append(node)
+
         return xml_view
 
+    @api.multi
+    def get_cfg_step(self):
+        self.ensure_one()
+        try:
+            cfg_step_id = int(self.state)
+        except:
+            cfg_step = self.env['product.config.step.line']
+        else:
+            cfg_step = self.product_tmpl_id.config_step_line_ids.filtered(
+                lambda x: x.id == cfg_step_id)
+        return cfg_step
+
     @api.model
     def create(self, vals):
         """Sets the configuration values of the product_id if given (if any).
-        This is used in reconfiguration of a existing variant"""
+        This is used in reconfiguration of an existing variant"""
         vals.update(user_id=self.env.uid)
 
         if 'product_id' in vals:
@@ -562,7 +654,13 @@ def read(self, fields=None, load='_classic_read'):
         ]
 
         dynamic_fields = attr_vals + custom_attr_vals
-        fields = [f for f in fields if f not in dynamic_fields]
+        support_fields = [
+            f for f in fields if f.startswith(self.ro_field_prefix) or
+            f.startswith(self.reqd_field_prefix) or
+            f.startswith(self.invis_field_prefix)
+        ]
+        fields = [
+            f for f in fields if f not in dynamic_fields + support_fields]
 
         custom_ext_id = 'product_configurator.custom_attribute_value'
         custom_val = self.env.ref(custom_ext_id)
@@ -610,6 +708,13 @@ def read(self, fields=None, load='_classic_read'):
                 except:
                     continue
             res[0].update(dynamic_vals)
+
+        if not self.env.context.get('from_fvg'):
+            for res_data in res:
+                if res_data.get('stored_support_vals'):
+                    res_data.update(
+                        ast.literal_eval(res_data['stored_support_vals'])
+                    )
         return res
 
     @api.multi
@@ -622,6 +727,20 @@ def write(self, vals):
         custom_ext_id = 'product_configurator.custom_attribute_value'
         custom_val = self.env.ref(custom_ext_id)
 
+        if vals.get('stored_support_vals'):
+            support_vals = ast.literal_eval(vals['stored_support_vals'])
+        else:
+            support_vals = {}
+        support_vals.update(
+            {k: v for k, v in vals.iteritems()
+             if k.startswith(self.ro_field_prefix) or
+             k.startswith(self.reqd_field_prefix) or
+             k.startswith(self.invis_field_prefix)
+             }
+        )
+        if support_vals:
+            vals.update({'stored_support_vals': str(support_vals)})
+
         attr_val_dict = {}
         custom_val_dict = {}
 
@@ -629,6 +748,18 @@ def write(self, vals):
             attr_id = attr_line.attribute_id.id
             field_name = self.field_prefix + str(attr_id)
             custom_field_name = self.custom_field_prefix + str(attr_id)
+            ro_field_name = self.ro_field_prefix + str(attr_id)
+            reqd_field_name = self.reqd_field_prefix + str(attr_id)
+            invis_field_name = self.invis_field_prefix + str(attr_id)
+
+            if ro_field_name in vals and vals.get(ro_field_name):
+                # readonly - must have an empty domain.  Ensure we
+                # store an empty value...
+                vals[field_name] = False
+                vals[custom_field_name] = False
+            vals.pop(ro_field_name, None)
+            vals.pop(reqd_field_name, None)
+            vals.pop(invis_field_name, None)
 
             if field_name not in vals and custom_field_name not in vals:
                 continue
@@ -646,7 +777,7 @@ def write(self, vals):
                     field_val = vals[field_name]
                 else:
                     raise Warning(
-                        _('An error occursed while parsing value for '
+                        _('An error occurred while parsing value for '
                           'attribute %s' % attr_line.attribute_id.name)
                     )
                 attr_val_dict.update({
@@ -672,10 +803,8 @@ def write(self, vals):
                 attr_val_dict.update({attr_id: False})
 
             # Remove dynamic field from value list to prevent error
-            if field_name in vals:
-                del vals[field_name]
-            if custom_field_name in vals:
-                del vals[custom_field_name]
+            vals.pop(field_name, None)
+            vals.pop(custom_field_name, None)
 
         self.config_session.update_config(attr_val_dict, custom_val_dict)
         res = super(ProductConfigurator, self).write(vals)
@@ -722,14 +851,7 @@ def action_next_step(self):
                 self.state = 'configure'
                 return wizard_action
 
-        try:
-            cfg_step_line_id = int(self.state)
-        except:
-            cfg_step_line_id = None
-
-        active_cfg_line_id = cfg_step_lines.filtered(
-            lambda x: x.id == cfg_step_line_id).id
-
+        active_cfg_line_id = self.get_cfg_step().id
         adjacent_steps = self.product_tmpl_id.get_adjacent_steps(
             self.value_ids.ids, active_cfg_line_id)
 
@@ -766,13 +888,7 @@ def action_previous_step(self):
         if not cfg_step_lines:
             return wizard_action
 
-        try:
-            cfg_step_line_id = int(self.state)
-            active_cfg_line_id = cfg_step_lines.filtered(
-                lambda x: x.id == cfg_step_line_id).id
-        except:
-            active_cfg_line_id = None
-
+        active_cfg_line_id = self.get_cfg_step().id
         adjacent_steps = self.product_tmpl_id.get_adjacent_steps(
             self.value_ids.ids, active_cfg_line_id)
 
diff --git a/product_configurator_wizard/wizard/product_configurator_view.xml b/product_configurator_wizard/wizard/product_configurator_view.xml
index 88580774..4368b2ed 100644
--- a/product_configurator_wizard/wizard/product_configurator_view.xml
+++ b/product_configurator_wizard/wizard/product_configurator_view.xml
@@ -21,6 +21,7 @@
                                     <field name="multi"/>
                                 </tree>
                             </field>
+                            <field name="stored_support_vals" invisible='1'/>
                         </group>
                         <group colspan="1">
                             <field attrs="{'invisible': [('product_img', '=', False)]}" name="product_img" readonly="1" nolabel="1" widget="image"/>

From 6ec99f6f7908f6e583eadba865817463a4c59b99 Mon Sep 17 00:00:00 2001
From: Richard deMeester <richard@willowit.com.au>
Date: Mon, 29 May 2017 18:08:04 +1000
Subject: [PATCH 2/6] Test enhancements, including some on new attributes.

---
 product_configurator_wizard/tests/__init__.py |   1 +
 .../tests/test_wizard.py                      |  69 ++++--
 .../tests/test_wizard_attrs.py                | 198 ++++++++++++++++++
 3 files changed, 253 insertions(+), 15 deletions(-)
 create mode 100644 product_configurator_wizard/tests/test_wizard_attrs.py

diff --git a/product_configurator_wizard/tests/__init__.py b/product_configurator_wizard/tests/__init__.py
index e037c216..404c70da 100644
--- a/product_configurator_wizard/tests/__init__.py
+++ b/product_configurator_wizard/tests/__init__.py
@@ -1,3 +1,4 @@
 # -*- coding: utf-8 -*-
 
 from . import test_wizard
+from . import test_wizard_attrs
diff --git a/product_configurator_wizard/tests/test_wizard.py b/product_configurator_wizard/tests/test_wizard.py
index 3acd8643..eaad1d16 100644
--- a/product_configurator_wizard/tests/test_wizard.py
+++ b/product_configurator_wizard/tests/test_wizard.py
@@ -23,11 +23,13 @@ def get_attr_values(self, attr_val_ext_ids=None):
 
         return attr_vals
 
-    def get_wizard_write_dict(self, wizard, attr_values):
+    def get_wizard_write_dict(self, wizard, attr_values, remove_values=None):
         """Turn a series of attribute.value objects to a dictionary meant for
         writing values to the product.configurator wizard"""
 
         write_dict = {}
+        if remove_values is None:
+            remove_values = []
 
         multi_attr_ids = wizard.product_tmpl_id.attribute_line_ids.filtered(
             lambda x: x.multi).mapped('attribute_id').ids
@@ -40,6 +42,10 @@ def get_wizard_write_dict(self, wizard, attr_values):
                 continue
             write_dict.update({field_name: val.id})
 
+        for val in remove_values:
+            field_name = wizard.field_prefix + str(val.attribute_id.id)
+            write_dict.update({field_name: False})
+
         return write_dict
 
     def wizard_write_proceed(self, wizard, attr_vals, value_ids=None):
@@ -54,6 +60,10 @@ def wizard_write_proceed(self, wizard, attr_vals, value_ids=None):
     def test_wizard_configuration(self):
         """Test product configurator wizard"""
 
+        config_variants = self.env['product.product'].search([
+            ('config_ok', '=', True)
+        ])
+
         # Start a new configuration wizard
         wizard_obj = self.env['product.configurator'].with_context({
             'active_model': 'sale.order',
@@ -87,37 +97,66 @@ def test_wizard_configuration(self):
 
         wizard.action_next_step()
 
-        config_variants = self.env['product.product'].search([
+        new_config_variants = self.env['product.product'].search([
             ('config_ok', '=', True)
         ])
 
-        self.assertTrue(len(config_variants) == 1,
+        self.assertTrue(len(new_config_variants - config_variants) == 1,
                         "Wizard did not create a configurable variant")
 
+    def do_reconfigure(self, order_line, attr_vals):
+        reconfig_action = order_line.reconfigure_product()
+
+        wizard = self.env['product.configurator'].browse(
+            reconfig_action.get('res_id')
+        )
+
+        self.wizard_write_proceed(wizard, attr_vals)
+
+        # if wizard only had one step, it would already have completed
+        if wizard.exists():
+            # Cycle through steps until wizard ends
+            while wizard.action_next_step():
+                pass
+
     def test_reconfiguration(self):
         """Test reconfiguration functionality of the wizard"""
+        config_variants = self.env['product.product'].search([
+            ('config_ok', '=', True)
+        ])
+
         self.test_wizard_configuration()
 
+        existing_lines = self.so.order_line
+
         order_line = self.so.order_line.filtered(
             lambda l: l.product_id.config_ok
         )
 
-        reconfig_action = order_line.reconfigure_product()
+        self.do_reconfigure(order_line,
+                            self.get_attr_values(['diesel', '220d'])
+                            )
 
-        wizard = self.env['product.configurator'].browse(
-            reconfig_action.get('res_id')
-        )
+        new_config_variants = self.env['product.product'].search([
+            ('config_ok', '=', True)
+        ])
 
-        attr_vals = self.get_attr_values(['diesel', '220d'])
-        self.wizard_write_proceed(wizard, attr_vals)
+        self.assertTrue(len(new_config_variants - config_variants) == 2,
+                        "Wizard reconfiguration did not create a new variant")
 
-        # Cycle through steps until wizard ends
-        while wizard.action_next_step():
-            pass
+        created_line = self.so.order_line - existing_lines
+        self.assertTrue(len(created_line) == 0,
+                        "Wizard created an order line on reconfiguration")
 
-        config_variants = self.env['product.product'].search([
+        # test that running through again with the same values does not
+        # create another variant
+        self.do_reconfigure(order_line,
+                            self.get_attr_values(['diesel', '220d'])
+                            )
+
+        new_config_variants = self.env['product.product'].search([
             ('config_ok', '=', True)
         ])
 
-        self.assertTrue(len(config_variants) == 2,
-                        "Wizard reconfiguration did not create a new variant")
+        self.assertTrue(len(new_config_variants - config_variants) == 2,
+                        "Wizard reconfiguration created a redundant variant")
diff --git a/product_configurator_wizard/tests/test_wizard_attrs.py b/product_configurator_wizard/tests/test_wizard_attrs.py
new file mode 100644
index 00000000..12f4b7b9
--- /dev/null
+++ b/product_configurator_wizard/tests/test_wizard_attrs.py
@@ -0,0 +1,198 @@
+# -*- coding: utf-8 -*-
+
+from odoo.addons.product_configurator_wizard.tests.test_wizard \
+    import ConfigurationRules
+
+
+class ConfigurationAttributes(ConfigurationRules):
+
+    def setUp(self):
+        """
+        Product with 3 sizes:
+            Small or Medium allow Blue or Red, but colour is optional
+            Large does not allow colour selection
+        """
+        super(ConfigurationAttributes, self).setUp()
+
+        self.attr_size = self.env['product.attribute'].create(
+            {'name': 'Size'})
+        self.attr_val_small = self.env['product.attribute.value'].create(
+            {'attribute_id': self.attr_size.id,
+             'name': 'Small',
+             }
+        )
+        self.attr_val_med = self.env['product.attribute.value'].create(
+            {'attribute_id': self.attr_size.id,
+             'name': 'Medium',
+             }
+        )
+        self.attr_val_large = self.env['product.attribute.value'].create(
+            {'attribute_id': self.attr_size.id,
+             'name': 'Large (Green)',
+             }
+        )
+        domain_small_med = self.env['product.config.domain'].create(
+            {'name': 'Small/Med',
+             'domain_line_ids': [
+                 (0, 0, {'attribute_id': self.attr_size.id,
+                         'condition': 'in',
+                         'operator': 'and',
+                         'value_ids':
+                             [(6, 0,
+                               [self.attr_val_small.id, self.attr_val_med.id]
+                               )]
+                         }),
+                 ],
+             }
+        )
+        self.attr_colour = self.env['product.attribute'].create(
+            {'name': 'Colour'})
+        self.attr_val_blue = self.env['product.attribute.value'].create(
+            {'attribute_id': self.attr_colour.id,
+             'name': 'Blue',
+             }
+        )
+        self.attr_val_red = self.env['product.attribute.value'].create(
+            {'attribute_id': self.attr_colour.id,
+             'name': 'Red',
+             }
+        )
+        self.product_temp = self.env['product.template'].create(
+            {'name': 'Config Product',
+             'config_ok': True,
+             'type': 'product',
+             'categ_id': self.env['ir.model.data'].xmlid_to_res_id(
+                 'product.product_category_5'
+             ),
+             'attribute_line_ids': [
+                 (0, 0, {'attribute_id': self.attr_size.id,
+                         'value_ids': [
+                             (6, 0, self.attr_size.value_ids.ids),
+                             ],
+                         'required': True,
+                         }),
+                 (0, 0, {'attribute_id': self.attr_colour.id,
+                         'value_ids': [
+                             (6, 0, self.attr_colour.value_ids.ids),
+                             ],
+                         'required': False,
+                         })
+                 ],
+             }
+        )
+        colour_line = self.product_temp.attribute_line_ids.filtered(
+            lambda a: a.attribute_id == self.attr_colour)
+        self.env['product.config.line'].create({
+            'product_tmpl_id': self.product_temp.id,
+            'attribute_line_id': colour_line.id,
+            'value_ids': [(6, 0, self.attr_colour.value_ids.ids)],
+            'domain_id': domain_small_med.id,
+            })
+
+    def test_configurations_option_or_not_reqd(self):
+        # Start a new configuration wizard
+        wizard_obj = self.env['product.configurator'].with_context({
+            'active_model': 'sale.order',
+            'active_id': self.so.id
+            # 'default_order_id': self.so.id
+        })
+
+        wizard = wizard_obj.create({'product_tmpl_id': self.product_temp.id})
+        wizard.action_next_step()
+
+        dynamic_fields = {}
+        for attribute_line in self.product_temp.attribute_line_ids:
+            field_name = '%s%s' % (
+                wizard.field_prefix,
+                attribute_line.attribute_id.id
+            )
+            dynamic_fields[field_name] = [] if attribute_line.multi else False
+        field_name_colour = '%s%s' % (
+            wizard.field_prefix,
+            self.attr_colour.id
+        )
+        ro_name_colour = '%s%s' % (
+            wizard.ro_field_prefix,
+            self.attr_colour.id
+        )
+        reqd_name_colour = '%s%s' % (
+            wizard.ro_field_prefix,
+            self.attr_colour.id
+        )
+
+        # Define small without colour specified
+        self.wizard_write_proceed(wizard, [self.attr_val_small])
+        new_variant = self.product_temp.product_variant_ids
+        self.assertTrue(len(new_variant) == 1 and
+                        set(new_variant.attribute_value_ids.ids) ==
+                        set([self.attr_val_small.id]),
+                        "Wizard did not accurately create a variant with "
+                        "optional value undefined")
+        config_variants = self.product_temp.product_variant_ids
+
+        order_line = self.so.order_line.filtered(
+            lambda l: l.product_id.config_ok
+        )
+
+        # Redefine to medium without colour
+        self.do_reconfigure(order_line, [self.attr_val_med])
+        new_variant = self.product_temp.product_variant_ids - config_variants
+        self.assertTrue(len(new_variant) == 1 and
+                        set(new_variant.attribute_value_ids.ids) ==
+                        set([self.attr_val_med.id]),
+                        "Wizard did not accurately reconfigure a variant with "
+                        "optional value undefined")
+        config_variants = self.product_temp.product_variant_ids
+
+        # Redefine to medium blue
+        self.do_reconfigure(order_line, [self.attr_val_blue])
+        new_variant = self.product_temp.product_variant_ids - config_variants
+        self.assertTrue(len(new_variant) == 1 and
+                        set(new_variant.attribute_value_ids.ids) ==
+                        set([self.attr_val_med.id, self.attr_val_blue.id]),
+                        "Wizard did not accurately reconfigure a variant with "
+                        "to add an optional value")
+        config_variants = self.product_temp.product_variant_ids
+
+        # Redefine to large - should remove colour, as this is invalid
+        reconfig_action = order_line.reconfigure_product()
+        wizard = self.env['product.configurator'].browse(
+            reconfig_action.get('res_id')
+        )
+        attr_large_dict = self.get_wizard_write_dict(wizard,
+                                                     [self.attr_val_large])
+        attr_blue_dict = self.get_wizard_write_dict(wizard,
+                                                    [self.attr_val_blue])
+        oc_vals = dynamic_fields.copy()
+        oc_vals.update({'id': wizard.id})
+        oc_vals.update(dict(attr_blue_dict, **attr_large_dict))
+        oc_result = wizard.onchange(
+            oc_vals,
+            attr_large_dict.keys()[0],
+            {}
+        )
+        self.assertTrue(field_name_colour in oc_result['value'] and
+                        not oc_result['value'][field_name_colour],
+                        "Colour should have been cleared by wizard"
+                        )
+        self.assertTrue(ro_name_colour in oc_result['value'] and
+                        oc_result['value'][ro_name_colour],
+                        "Colour should have become readonly"
+                        )
+        self.assertTrue(oc_result['value'].get(reqd_name_colour),
+                        "Colour should not have become required"
+                        )
+
+        vals = self.get_wizard_write_dict(wizard, [self.attr_val_large],
+                                          remove_values=[self.attr_val_blue])
+        wizard.write(vals)
+        wizard.action_next_step()
+        if wizard.exists():
+            while wizard.action_next_step():
+                pass
+        new_variant = self.product_temp.product_variant_ids - config_variants
+        self.assertTrue(len(new_variant) == 1 and
+                        set(new_variant.attribute_value_ids.ids) ==
+                        set([self.attr_val_large.id]),
+                        "Wizard did not accurately reconfigure a variant "
+                        "to remove invalid attribute")

From f25afad67d047c0dbc6d89fbfa916937c1190957 Mon Sep 17 00:00:00 2001
From: Richard deMeester <richard@willowit.com.au>
Date: Mon, 29 May 2017 18:38:40 +1000
Subject: [PATCH 3/6] Flake 8 changes

---
 product_configurator_wizard/wizard/product_configurator.py | 1 -
 1 file changed, 1 deletion(-)

diff --git a/product_configurator_wizard/wizard/product_configurator.py b/product_configurator_wizard/wizard/product_configurator.py
index afe2e08a..4eb02e91 100644
--- a/product_configurator_wizard/wizard/product_configurator.py
+++ b/product_configurator_wizard/wizard/product_configurator.py
@@ -309,7 +309,6 @@ def fields_get(self, allfields=None, attributes=None):
 
         # Get the wizard object from the database
         wiz = self.browse(wizard_id)
-        active_step_id = wiz.state
 
         # If the product template is not set it is still at the 1st step
         if not wiz.product_tmpl_id:

From eebeb994c4fff4b7aeb67d180ff1f0b0ebe7545f Mon Sep 17 00:00:00 2001
From: Richard deMeester <richard@willowit.com.au>
Date: Mon, 29 May 2017 19:41:53 +1000
Subject: [PATCH 4/6] More tests and test fix.

---
 .../tests/test_wizard_attrs.py                | 39 +++++++++++++++----
 1 file changed, 31 insertions(+), 8 deletions(-)

diff --git a/product_configurator_wizard/tests/test_wizard_attrs.py b/product_configurator_wizard/tests/test_wizard_attrs.py
index 12f4b7b9..669c1ea0 100644
--- a/product_configurator_wizard/tests/test_wizard_attrs.py
+++ b/product_configurator_wizard/tests/test_wizard_attrs.py
@@ -9,7 +9,7 @@ class ConfigurationAttributes(ConfigurationRules):
     def setUp(self):
         """
         Product with 3 sizes:
-            Small or Medium allow Blue or Red, but colour is optional
+            Small or Medium allow Blue or Red, colour is required
             Large does not allow colour selection
         """
         super(ConfigurationAttributes, self).setUp()
@@ -80,11 +80,12 @@ def setUp(self):
                  ],
              }
         )
-        colour_line = self.product_temp.attribute_line_ids.filtered(
-            lambda a: a.attribute_id == self.attr_colour)
+        self.template_colour_line = \
+            self.product_temp.attribute_line_ids.filtered(
+                lambda a: a.attribute_id == self.attr_colour)
         self.env['product.config.line'].create({
             'product_tmpl_id': self.product_temp.id,
-            'attribute_line_id': colour_line.id,
+            'attribute_line_id': self.template_colour_line.id,
             'value_ids': [(6, 0, self.attr_colour.value_ids.ids)],
             'domain_id': domain_small_med.id,
             })
@@ -116,7 +117,7 @@ def test_configurations_option_or_not_reqd(self):
             self.attr_colour.id
         )
         reqd_name_colour = '%s%s' % (
-            wizard.ro_field_prefix,
+            wizard.reqd_field_prefix,
             self.attr_colour.id
         )
 
@@ -179,9 +180,9 @@ def test_configurations_option_or_not_reqd(self):
                         oc_result['value'][ro_name_colour],
                         "Colour should have become readonly"
                         )
-        self.assertTrue(oc_result['value'].get(reqd_name_colour),
-                        "Colour should not have become required"
-                        )
+        self.assertFalse(oc_result['value'].get(reqd_name_colour, False),
+                         "Colour should not have become required"
+                         )
 
         vals = self.get_wizard_write_dict(wizard, [self.attr_val_large],
                                           remove_values=[self.attr_val_blue])
@@ -196,3 +197,25 @@ def test_configurations_option_or_not_reqd(self):
                         set([self.attr_val_large.id]),
                         "Wizard did not accurately reconfigure a variant "
                         "to remove invalid attribute")
+
+        # Now test reqd attribute changes.
+        self.template_colour_line.write({'required': True})
+
+        # Redefine to medium - should make colour required
+        reconfig_action = order_line.reconfigure_product()
+        wizard = self.env['product.configurator'].browse(
+            reconfig_action.get('res_id')
+        )
+        attr_med_dict = self.get_wizard_write_dict(wizard,
+                                                   [self.attr_val_med])
+        oc_vals = dynamic_fields.copy()
+        oc_vals.update({'id': wizard.id})
+        oc_vals.update(attr_med_dict)
+        oc_result = wizard.onchange(
+            oc_vals,
+            attr_med_dict.keys()[0],
+            {}
+        )
+        self.assertTrue(oc_result['value'].get(reqd_name_colour),
+                        "Colour should have become required"
+                        )

From eb7da50c9a06bc4f3f2499627bad218d51566beb Mon Sep 17 00:00:00 2001
From: Richard deMeester <richard@willowit.com.au>
Date: Mon, 29 May 2017 20:04:50 +1000
Subject: [PATCH 5/6] Even more tests

---
 .../tests/test_wizard_attrs.py                       | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/product_configurator_wizard/tests/test_wizard_attrs.py b/product_configurator_wizard/tests/test_wizard_attrs.py
index 669c1ea0..fc4728f4 100644
--- a/product_configurator_wizard/tests/test_wizard_attrs.py
+++ b/product_configurator_wizard/tests/test_wizard_attrs.py
@@ -199,6 +199,7 @@ def test_configurations_option_or_not_reqd(self):
                         "to remove invalid attribute")
 
         # Now test reqd attribute changes.
+        last_variant = new_variant
         self.template_colour_line.write({'required': True})
 
         # Redefine to medium - should make colour required
@@ -219,3 +220,14 @@ def test_configurations_option_or_not_reqd(self):
         self.assertTrue(oc_result['value'].get(reqd_name_colour),
                         "Colour should have become required"
                         )
+        # Redefine to large - should make colour not required, despite master
+        # file
+        vals = self.get_wizard_write_dict(wizard, [self.attr_val_large])
+        wizard.write(vals)
+        wizard.action_next_step()
+        if wizard.exists():
+            while wizard.action_next_step():
+                pass
+        self.assertTrue(order_line.product_id == last_variant,
+                        "Wizard did not end up with the same "
+                        "variant")

From e2a67d23ed49d9a0fdf5f42824d01be3b5ed69bd Mon Sep 17 00:00:00 2001
From: Richard deMeester <richard@willowit.com.au>
Date: Thu, 1 Jun 2017 20:37:28 +1000
Subject: [PATCH 6/6] Changes as requested.

---
 .../wizard/product_configurator.py            | 52 +++++++++++++------
 1 file changed, 36 insertions(+), 16 deletions(-)

diff --git a/product_configurator_wizard/wizard/product_configurator.py b/product_configurator_wizard/wizard/product_configurator.py
index 4eb02e91..90501188 100644
--- a/product_configurator_wizard/wizard/product_configurator.py
+++ b/product_configurator_wizard/wizard/product_configurator.py
@@ -113,17 +113,30 @@ def get_onchange_domains(self, values, cfg_val_ids):
                     continue
         return domains
 
-    def get_support_fields(self, k, available_val_ids, tmpl_attribute):
-        vals = {}
+    def get_support_fields(self, k, available_val_ids, attribute_line):
+        """ Sets the readonly, required, and invisible boolean values
+        which drive the behaviour of individual fields.
+
+        :param k: the field name of the dynamic field
+        :param available_val_ids: list of the possible value ids for the field
+        :param attribute_line: corresponding attribute line definition from the
+        product template
+
+        :returns vals: a dictionary with three values
+        """
         attribute_id = int(k.replace(self.field_prefix, ''))
-        vals[self.ro_field_prefix + str(attribute_id)] = \
-            len(available_val_ids) == 0
-        if tmpl_attribute.required:
+        ro_field = self.ro_field_prefix + str(attribute_id)
+        reqd_field = self.reqd_field_prefix + str(attribute_id)
+        invis_field = self.invis_field_prefix + str(attribute_id)
+
+        vals = {}
+        vals[ro_field] = len(available_val_ids) == 0
+        if attribute_line.required:
             required = len(available_val_ids) > 0
         else:
             required = False
-        vals[self.reqd_field_prefix + str(attribute_id)] = required
-        vals[self.invis_field_prefix + str(attribute_id)] = False
+        vals[reqd_field] = required
+        vals[invis_field] = False
         return vals
 
     def get_form_vals(self, dynamic_fields, domains, cfg_step):
@@ -142,12 +155,12 @@ def get_form_vals(self, dynamic_fields, domains, cfg_step):
 
         for k, v in dynamic_fields.iteritems():
             attribute_id = int(k.replace(self.field_prefix, ''))
-            tmpl_attribute = self.product_tmpl_id.attribute_line_ids.filtered(
+            attribute_line = self.product_tmpl_id.attribute_line_ids.filtered(
                 lambda a: a.attribute_id.id == attribute_id)
             available_val_ids = domains[k][0][2]
 
             vals.update(
-                self.get_support_fields(k, available_val_ids, tmpl_attribute)
+                self.get_support_fields(k, available_val_ids, attribute_line)
             )
 
             if v and isinstance(v, list):
@@ -159,7 +172,7 @@ def get_form_vals(self, dynamic_fields, domains, cfg_step):
                 vals[k] = None
                 v = None
             if not v:
-                if tmpl_attribute.required and not tmpl_attribute.multi and \
+                if attribute_line.required and not attribute_line.multi and \
                         len(available_val_ids) == 1:
                     dynamic_fields.update({k: available_val_ids[0]})
                     vals[k] = available_val_ids[0]
@@ -224,9 +237,9 @@ def onchange(self, values, field_name, field_onchange):
 
         # See if any of the dynamic_fields were modified by the get_form_vals
         # (cleared or set), they may change domains!
-        modified_dynamics = {k: v
-                             for k, v in vals.iteritems()
-                             if k in dynamic_fields}
+        modified_dynamics = {
+            k: v for k, v in vals.iteritems() if k in dynamic_fields
+        }
         while modified_dynamics:
             dynamic_fields.update(modified_dynamics)
             for k, v in modified_dynamics.iteritems():
@@ -247,11 +260,16 @@ def onchange(self, values, field_name, field_onchange):
             nvals = self.get_form_vals(dynamic_fields, domains, cfg_step)
             # Stop possible recursion by not including values which have
             # previously looped
-            modified_dynamics = {k: v
-                                 for k, v in nvals.iteritems()
-                                 if k in dynamic_fields and k not in vals}
+            modified_dynamics = {
+                k: v for k, v in nvals.iteritems()
+                if k in dynamic_fields and k not in vals
+            }
             vals.update(nvals)
 
+        # To ensure the full suite of support vals is written if any are
+        # changed by onchange.
+        if self.stored_support_vals:
+            vals['stored_support_vals'] = self.stored_support_vals + ' '
         return {'value': vals, 'domain': domains}
 
     attribute_line_ids = fields.One2many(
@@ -610,6 +628,8 @@ def add_dynamic_fields(self, res, dynamic_fields, support_fields, wiz):
 
     @api.multi
     def get_cfg_step(self):
+        """Attempt to return product.config.step.line object which
+        is encoded in the wizard state string"""
         self.ensure_one()
         try:
             cfg_step_id = int(self.state)