Skip to content
This repository has been archived by the owner on Apr 28, 2022. It is now read-only.

[ENH] 10.0 raise warnings #84

Open
wants to merge 2 commits into
base: 10.0
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion product_configurator/demo/product_attribute.xml
Original file line number Diff line number Diff line change
Expand Up @@ -335,7 +335,7 @@
ref('product_configurator.product_attribute_value_tow_hook'),
]
)]"/>
<field name="required" eval="True"/>
<field name="required" eval="False"/>
<field name="multi" eval="True"/>
</record>

Expand Down
3 changes: 3 additions & 0 deletions product_configurator/demo/product_config_lines.xml
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@
ref('product_attribute_value_m235i'),
ref('product_attribute_value_m235i_xdrive')])]"/>
<field name="domain_id" ref="product_config_domain_gasoline"/>
<field name="rule_description">Gasoline Fuel requires a Gasoline Engine to be selected</field>
</record>

<record id="product_config_line_diesel_engines" model="product.config.line">
Expand All @@ -22,6 +23,7 @@
ref('product_attribute_value_220d_xdrive'),
ref('product_attribute_value_225d')])]"/>
<field name="domain_id" ref="product_config_domain_diesel"/>
<field name="rule_description">Diesel Fuel requires a Diesel Engine to be selected</field>
</record>

<record id="product_config_line_218_lines" model="product.config.line">
Expand All @@ -31,6 +33,7 @@
ref('product_attribute_value_sport_line'),
ref('product_attribute_value_luxury_line')])]"/>
<field name="domain_id" ref="product_config_domain_218_engine"/>
<field name="rule_description">218i Engine has a limited selection of Lines</field>
</record>

<record id="product_config_line_luxury_lines" model="product.config.line">
Expand Down
44 changes: 36 additions & 8 deletions product_configurator/models/product.py
Original file line number Diff line number Diff line change
Expand Up @@ -357,7 +357,8 @@ def create_get_variant(self, value_ids, custom_values=None):
"""
if custom_values is None:
custom_values = {}
valid = self.validate_configuration(value_ids, custom_values)
valid = self.validate_configuration(value_ids, custom_values,
do_raise=True)
if not valid:
raise ValidationError(_('Invalid Configuration'))

Expand Down Expand Up @@ -413,14 +414,15 @@ def validate_domains_against_sels(self, domains, sel_val_ids):
return avail

@api.multi
def values_available(self, attr_val_ids, sel_val_ids):
def values_available(self, attr_val_ids, sel_val_ids, do_raise=False):
"""Determines whether the attr_values from the product_template
are available for selection given the configuration ids and the
dependencies set on the product template

:param attr_val_ids: list of attribute value ids to check for
availability
:param sel_val_ids: list of attribute value ids already selected
:param do_raise: boolean on whether to raise a warning or just fail

:returns: list of available attribute values
"""
Expand All @@ -436,24 +438,36 @@ def values_available(self, attr_val_ids, sel_val_ids):
avail = self.validate_domains_against_sels(domains, sel_val_ids)
if avail:
avail_val_ids.append(attr_val_id)

elif do_raise:
if len(config_lines) == 1 and config_lines[0].rule_description:
Copy link
Contributor

@PCatinean PCatinean May 31, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This has pretty limited applicability, maybe we can find a better way to raise all the errors one by one as they are fixed or drop it I think

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Our use customer's use case is:

They configures the product on a single view, with around 15 options. Some of the rules and dependencies were too complicated to reflect in readonly's on the view (the other PR will fix a lot of that). Then they clicked submit and they just got a "configuration is invalid - please check all the steps" error, which did not help them focus on the problem.

The real benefit of this approach is where a configurable product has been defined badly. A value on step 3 may make a value on step 1 invalid! This is a bad setup. But, the rendering of the page cannot foresee this (and nor should it have to). If they save the configuration after step 3 in this scenario, it just says "please check all your steps", but with a focussed error message, they will see ("Value X should not be chosen when the product is Blue or Red")....

raise ValidationError(config_lines[0].rule_description)
else:
attribute_value = \
self.env['product.attribute.value'].browse(attr_val_id)
raise ValidationError(
_('%s for %s is not valid in this configuration') %
(attribute_value.name,
attribute_value.attribute_id.name
)
)
return avail_val_ids

@api.multi
def validate_configuration(self, value_ids, custom_vals=None, final=True):
def validate_configuration(self, value_ids,
custom_vals=None, final=True, do_raise=False):
""" Verifies if the configuration values passed via value_ids and custom_vals
are valid

:param value_ids: list of attribute value ids
:param custom_vals: custom values dict {attr_id: custom_val}
:param final: boolean marker to check required attributes.
pass false to check non-final configurations
:param do_raise: boolean on whether to raise a warning or just fail

:returns: Error dict with reason of validation failure
or True
"""
# TODO: Raise ConfigurationError with reason
# Check if required values are missing for final configuration
# TODO: Check if required values are missing for final configuration
if custom_vals is None:
custom_vals = {}

Expand All @@ -466,19 +480,33 @@ 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:
if do_raise:
raise ValidationError(_("No value provided for %s") %
line.attribute.name)
# 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)
avail_val_ids = self.values_available(value_ids, value_ids,
do_raise=do_raise)
if set(value_ids) - set(avail_val_ids):
return False

# Check if custom values are allowed
custom_attr_ids = self.attribute_line_ids.filtered(
'custom').mapped('attribute_id').ids

if not set(custom_vals.keys()) <= set(custom_attr_ids):
invalid_custom_vals = set(custom_vals.keys()) - set(custom_attr_ids)
if invalid_custom_vals:
if do_raise:
ProductAttribute = self.env['product.attribute']
attributes_names = ', '.join(attr.name
for attr in
ProductAttribute.browse(
list(invalid_custom_vals)))
raise ValidationError(
_('Attributes (%s) cannot hold custom values') %
attributes_names)
return False

# Check if there are multiple values passed for non-multi attributes
Expand Down
9 changes: 7 additions & 2 deletions product_configurator/models/product_config.py
Original file line number Diff line number Diff line change
Expand Up @@ -180,6 +180,11 @@ def onchange_attribute(self):
string='Restrictions'
)

rule_description = fields.Char(
string='Rule Description',
help='User Displayed error if rule is broken'
)

sequence = fields.Integer(string='Sequence', default=10)

_order = 'product_tmpl_id, sequence, id'
Expand Down Expand Up @@ -229,7 +234,7 @@ def _check_value_ids(self):
if not valid:
raise ValidationError(
_("Values entered for line '%s' generate "
"a incompatible configuration" % cfg_img.name)
"an incompatible configuration" % cfg_img.name)
)


Expand Down Expand Up @@ -451,7 +456,7 @@ def write(self, vals):
for x in self.custom_value_ids
}
valid = self.product_tmpl_id.validate_configuration(
self.value_ids.ids, custom_val_dict, final=False)
self.value_ids.ids, custom_val_dict, final=False, do_raise=True)
if not valid:
raise ValidationError(_('Invalid Configuration'))
return res
Expand Down
5 changes: 5 additions & 0 deletions product_configurator/tests/test_configuration_rules.py
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
# -*- coding: utf-8 -*-

from odoo.tests.common import TransactionCase
from odoo.exceptions import ValidationError


class ConfigurationRules(TransactionCase):
Expand Down Expand Up @@ -56,6 +57,10 @@ def test_invalid_configuration(self):
validation = self.cfg_tmpl.validate_configuration(attr_val_ids)
self.assertFalse(validation, "Incompatible values (Diesel Fuel -> "
"Gasoline Engine) configuration passed validation")
self.assertRaises(ValidationError,
self.cfg_tmpl.validate_configuration,
attr_val_ids, do_raise=True,
)

def test_missing_val_configuration(self):
conf = [
Expand Down
1 change: 1 addition & 0 deletions product_configurator/views/product_view.xml
Original file line number Diff line number Diff line change
Expand Up @@ -63,6 +63,7 @@
options="{'no_create': True, 'no_create_edit': True}"
context="{'show_attribute': False}"/>
<field name="domain_id"/>
<field name="rule_description" />
</tree>
</field>
<separator colspan="4" string="Configuration Steps"/>
Expand Down